[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] Initial implementation of a mpeg1 layer2 stream
From: |
malc |
Subject: |
Re: [Qemu-devel] [PATCH] Initial implementation of a mpeg1 layer2 streaming audio driver. |
Date: |
Sun, 7 Nov 2010 21:34:53 +0300 (MSK) |
User-agent: |
Alpine 2.00 (LNX 1167 2008-08-23) |
On Sun, 7 Nov 2010, Fran?ois Revol wrote:
>
> Le 7 nov. 2010 ? 19:09, malc a ?crit :
>
> > On Sun, 7 Nov 2010, Fran?ois Revol wrote:
> >
> > Please CC audio related stuff to audio maintainer.
>
> And that'd be you according to MAINTAINERS ?
>
> >> +static const char http_header[] = "HTTP/1.1 200 OK\r\nServer:
> >> QEMU\r\nContent-Type: audio/mpeg\r\n\r\n";
> >
> > Line is too long.
>
> Ok I'll break at 80col.
>
> >> + if (twolame->fd > -1)
> >> + return;
> >
> > Style.
>
> That is ?
Braces around statements.
> >> +
> >> + int csock = qemu_accept(twolame->lsock, (struct sockaddr *)&addr,
> >> &addrlen);
> >
> > C99 intermixed declartion and initialization is not allowed.
>
> This line I copied form ui/vnc.c which does violate C89 too btw...
I do not maintain ui/vnc.c.
>
> >> +
> >> + again:
> >> + if (twolame->fd > -1) {
> >> + written = write (twolame->fd, twolame->mpg_buf,
> >> converted);
> >> + if (written == -1) {
> >> + if (errno == EPIPE) {
> >> + dolog ("Lost peer\n");
> >> + close (twolame->fd);
> >> + twolame->fd = -1;
> >> + goto again;
> >
> > This goto is obfuscated.
>
> Not much more than in esdaudio.c
>
Yes much more, esdaudio doesn't close the descriptor before jumping.
>
> >> + }
> >> + obt_as.endianness = AUDIO_HOST_ENDIANNESS;
> >> +
> >> + audio_pcm_init_info (&hw->info, &obt_as);
> >> +
> >> + twolame_set_mode(twolame->options, (as->nchannels == 2) ?
> >> TWOLAME_STEREO : TWOLAME_MONO);
> >> + twolame_set_num_channels(twolame->options, as->nchannels);
> >> + twolame_set_in_samplerate(twolame->options, as->freq);
> >> + twolame_set_out_samplerate(twolame->options, as->freq);
> >> + twolame_set_bitrate(twolame->options, 160); //XXX:conf.
> >> +
> >> + if (twolame_init_params(twolame->options)) {
> >> + dolog ("Could not set twolame options\n");
> >> + return -1;
> >> + }
> >
> > Inconsistent space before opening paren.
>
> Sorry, used to the Haiku style without space before but it seemed to be
> different around.
>
>
> >> + twolame->mpg_buf = audio_calloc (AUDIO_FUNC, hw->samples, 1 <<
> >> hw->info.shift);
> >> + if (!twolame->mpg_buf) {
> >
> > pcm_buf is not freed.
> >
> >> +
> >> +// fail1:
> >
> > Do not use C99 style comments.
>
> Oh that's leftover from copied error handling, which isn't correct anyway.
>
> Fran?ois.
>
--
mailto:address@hidden