[FFmpeg-devel] [PATCH]: Modify decode_frame() in mpegaudiodec.c to only consume s->frame_size bytes max

[Chris Pinkham]

* On Sun May 27, 2007 at 11:17:40AM +0200, Michael Niedermayer wrote:
> i agree that s->frame_size should be returned instead of buf_size
> besides that your solution isnt correct (in the sense that it hides
> a bug rather then fixing it, not that it would neccessarily not work)
> 
> the issue is that libav* expects a single mp3 frame in a AVPacket
> and not multiple ones, later would break stream copy into other
> container formats which per specification need 1 mp3 frame within
> their packets.

OK, I was looking at the AC3 decoder and it appeared to be able to
consume multiple frames so I just took it for granted about the
mpeg audio decoder.

> i dunno if .nuv really was supposed to contain multiple
> mp3 frames per packet or if that was rather some encoder bug ...
> also mp3 frames can differ in their size (even for CBR mp3 when the
> ideal frame size is not an exact integer)

Yeah, I'm not sure about that.  I don't see anyway in the Myth code
where we could be appending 2 frames, so I'm wondering if lame is
giving us 2 frames somehow when we call lame_encode_buffer_interleaved().
I'll have to do some checking on this.  I agree that this needs to be
fixed.

> ive fixed the multiple mp3 frames in one AVPacket bug, a patch which makes
> decode_frame() return s->frame_size and properly decodes the first frame
> is welcome though (but the warning should be left there ...)
> 
> [...]

I think that it does properly decode the first frame, it's just that it
skips the second one and then tells the caller that it consumed all the
data in the buffer.  So, the audio ends up running fast on the player
because there is certain audio data that is being skipped and never even
decoded.

> 
> >          av_log(avctx, AV_LOG_ERROR, "incorrect frame size\n");
> >      }
> >  
> > -    out_size = mp_decode_frame(s, out_samples, buf, buf_size);
> > +    out_size = mp_decode_frame(s, out_samples, buf, s->frame_size);
> 
> why?

My assumption was because mp_decode_frame could operate on anything you
give it and if we know the frame should be only s->frame_size bytes then
why tell mp_decode_frame that we have more data.  This could be a totally
invalid assumption though, I'm not at all familiar with the internals of
mpeg encoding/decoding so my assumption about frame sizes may be incorrect
like what you mention above about the fact that they can differ.

So, if mp_decode_frame can consume varying amounts of info, then maybe
it needs to also return how much it consumed and that value should get
returned to decode_frame's caller.

> > +
> > +    int ret = s->frame_size;
> >      s->frame_size = 0;
> > -    return buf_size;
> > +    return ret;
> 
> mixing declarations and statements will break gcc 2.95

Yeah, forgot I upgraded this old old old RH9 box to gcc 3.2.2 ages ago. :)
Was trying to get my mods working at 2AM.

--
Chris