[MPlayer-dev-eng] [PATCH] use avcodec_decode_audio2

[Uoti Urpala]

On Tue, 2007-03-20 at 15:43 +0100, Reimar Döffinger wrote:
> On Tue, Mar 20, 2007 at 04:00:57PM +0200, Uoti Urpala wrote:
> > On Tue, 2007-03-20 at 12:50 +0100, Reimar Döffinger wrote:
> > > Sorry, previous patch was only correct on the first loop iteration,
> > > attached one might actually be correct (though either is an
> > > improvement over the current situation with no checking at all).
> > 
> > The current situation should be safe since dec_audio.c guarantees that
> > maxlen >= minlen+sh_audio->audio_out_minsize. So decoders can safely
> > ignore maxlen if they set audio_out_minsize to a suitable value.
> 
> I didn't mean to imply it is not safe, more like that it is suboptimal
> that everything working right depends on half of mplayer.c being
> correct.

It does not "depend on half of mplayer.c being correct". I added an
explicit check in dec_audio.c which makes sure that the condition is
true whenever a decoder is called.

> Also maxlen >= minlen+sh_audio->audio_out_minsize is neither an obvious
> condition nor one mentioned (in the almost non-existent) API
> documentation, so any change that makes it work without this assumption
> is an improvement...

IMO not having explicit documentation is not a reason to make the code
more complicated by not relying on sane behavior of other code (I don't
claim this particular change would make ad_ffmpeg more complicated, but
I wouldn't like changes that just limit decoding based on maxlen). If
you don't trust audio_out_minsize to work then the whole field becomes
rather useless. And if you want to add extra safety then IMO it should
be in the form of directly testing the condition and abort()ing if a bug
is detected, or at least printing a clear diagnostic if it's certain the
error can be safely ignored in this case.

> > > Some testing would be welcome, since the function currently still
> > > requires the buffer to be at least AVCODEC_MAX_AUDIO_FRAME_SIZE bytes
> > > big whether the codec needs it or not it _might_ cause some regressions.
> > 
> > As explained above there should be no way for avcodec_decode_audio2 to
> > get called with less buffer space than AVCODEC_MAX_AUDIO_FRAME_SIZE
> > (unless some avcodec decoder now returns more data than that during an
> > earlier call, but in practice even that would mean the loop just
> > terminates because that amount of data would almost certainly be over
> > minlen).
> 
> Well, there are always "if"s and "should"s. My feeling is that in this
> case there are a few more than usual, and testing can't hurt. It's not a
> big deal though.

To me the potential "if"s related to buffer sizes look either completely
theoretical or severe problems in existing code which would be better
detected quickly even if by breakage in real use.