[FFmpeg-devel] [PATCH 3/7] lavc/utils: check av_frame_alloc() failure.

[Nicolas George]

Le decadi 10 nivôse, an CCXXII, Michael Niedermayer a écrit :
> i test almost all my comits but its quite possible that what i tested
> (fate probably) did never test the code i changed, thats my mistake
> no question about that

I never doubted you did test, it was intended as a joke. The code is indeed
certainly not covered by FATE, or we would probably have noticed the
breakage I am reporting sooner, but your commit was perfectly correct
(except for the error check part, and this is of dubious use due to
overcommit).

> I think we should add some fate tests for old APIs
> maybe the examples could be adapted to do that, i could look into that
> but as you tested the code, i assume you already have a test for them?

I had the same idea.

> I possibly worded this unclearly
> what i meant was that if setting
> release_buffer= avcodec_default_release_buffer
> causes some problem then an application that calls
> avcodec_default_release_buffer from its release_buffer implementation
> would also cause the problem, and not setting release_buffer would
> likely leak and i saw one case where its called without a null
> pointer check so it could crash
> 
> but maybe iam missing something, can you elaborate about the issue
> you found ?

I suppose you may be forgetting something: since avcodec_decode_audio4() was
introduced (2011-09-06 / 0eea21294), using a custom get_buffer with compat
avcodec_decode_audio3() does not work and is forbidden. The original check
was:

+    if (avctx->get_buffer != avcodec_default_get_buffer) {
+        av_log(avctx, AV_LOG_ERROR, "A custom get_buffer() cannot be used with "
+               "avcodec_decode_audio3()\n");
+        return AVERROR(EINVAL);
+    }

And it was later amended to make the message clearer and override the custom
callback instead of failing (2012-01-15 / e2ff436ef):

+        av_log(avctx, AV_LOG_ERROR, "Custom get_buffer() for use with"
+               "avcodec_decode_audio3() detected. Overriding with
avcodec_default_get_buffer\n");
+        av_log(avctx, AV_LOG_ERROR, "Please port your application to "
+               "avcodec_decode_audio4()\n");
+        avctx->get_buffer = avcodec_default_get_buffer;

Unfortunately, since the refcounted frames, i.e. "the evil plan"
(2012-11-21, 759001c), the codec context initialization no longer sets the
get_buffer field (nor release_buffer, of course).

-    s->get_buffer          = avcodec_default_get_buffer;
-    s->release_buffer      = avcodec_default_release_buffer;
+    s->get_buffer2         = avcodec_default_get_buffer2;

Note that the fork ignores the problem since they dropped
FF_API_OLD_DECODE_AUDIO at the 54->55 version bump (refcounted frames),
while the test in FFmpeg is currently for the 56 bump.

I realize it was a bit exaggerated to state that avcodec_decode_audio3() is
broken: it only prints an obnoxious warning. The patch I posted removes that
warning by initing the get_buffer, but only when needed (no point in leaving
an obsolete field loaded unless necessary). I realize this is strange it did
work since I did not address the release_buffer part.

Regards,

-- 
  Nicolas George
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20131230/51f7ad57/attachment.asc>