diff mbox series

[FFmpeg-devel,v3] avcodec/mediacodecdec: Do not abort when H264/HEVC extradata extraction fails

Message ID DB6PR0901MB1495AC3DF11A248662226354EC8B9@DB6PR0901MB1495.eurprd09.prod.outlook.com
State New
Headers show
Series [FFmpeg-devel,v3] avcodec/mediacodecdec: Do not abort when H264/HEVC extradata extraction fails | expand

Checks

Context Check Description
andriy/x86_make success Make finished
andriy/x86_make_fate success Make fate finished
andriy/PPC64_make success Make finished
andriy/PPC64_make_fate success Make fate finished

Commit Message

sfan5 Feb. 12, 2021, 10:47 p.m. UTC 
12.02.21 - 21:43 - Andreas Rheinhardt:
> sfan5:
>> Hi,
>>
>>
>> attached v2 patch after discussion on IRC with JEEB (as he already
>> mentioned).
>>
>> Only change is that the log level turns to debug when missing parameter
>> sets are within spec (cf. 14496-15).
>>
>>
>> -        av_log(avctx, AV_LOG_ERROR, "Could not extract PPS/SPS from extradata");
>> -        ret = AVERROR_INVALIDDATA;
>> +        const int warn = is_avc && avctx->codec_tag != MKTAG('a','v','c','1') &&
>> +            avctx->codec_tag != MKTAG('a','v','c','2');
>> +        av_log(avctx, warn ? AV_LOG_WARNING : AV_LOG_DEBUG,
>> +            "Could not extract PPS/SPS from extradata\n");
>> +        ret = 0;
>>       }
> warn = is_avc && (avctx->codec_tag == MKTAG('a','v','c','1') ||
>                    avctx->codec_tag == MKTAG('a','v','c','2')
> is what you (should) want.
>
> - Andreas

Thanks for pointing that out, you're correct.

here's v3:
Subject: [PATCH v3] avcodec/mediacodecdec: Do not abort when H264/HEVC
 extradata extraction fails

Although rare, extradata can be present but empty and extraction will fail.
However Android also supports passing codec-specific data inline and
will likely play such a stream anyway. So there's no reason to abort
initialization before we know for sure.
---
 libavcodec/mediacodecdec.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

Comments

Jan Ekström Feb. 13, 2021, 2:31 p.m. UTC | #1 
On Sat, Feb 13, 2021 at 12:48 AM sfan5 <sfan5@live.de> wrote:
>
> 12.02.21 - 21:43 - Andreas Rheinhardt:
> > sfan5:
> >> Hi,
> >>
> >>
> >> attached v2 patch after discussion on IRC with JEEB (as he already
> >> mentioned).
> >>
> >> Only change is that the log level turns to debug when missing parameter
> >> sets are within spec (cf. 14496-15).
> >>
> >>
> >> -        av_log(avctx, AV_LOG_ERROR, "Could not extract PPS/SPS from extradata");
> >> -        ret = AVERROR_INVALIDDATA;
> >> +        const int warn = is_avc && avctx->codec_tag != MKTAG('a','v','c','1') &&
> >> +            avctx->codec_tag != MKTAG('a','v','c','2');
> >> +        av_log(avctx, warn ? AV_LOG_WARNING : AV_LOG_DEBUG,
> >> +            "Could not extract PPS/SPS from extradata\n");
> >> +        ret = 0;
> >>       }
> > warn = is_avc && (avctx->codec_tag == MKTAG('a','v','c','1') ||
> >                    avctx->codec_tag == MKTAG('a','v','c','2')
> > is what you (should) want.
> >
> > - Andreas
>
> Thanks for pointing that out, you're correct.
>
> here's v3:

After some brief discussion on IRC, this looks good to me :) .

Jan
Jan Ekström Feb. 14, 2021, 11:52 a.m. UTC | #2 
On Sat, Feb 13, 2021 at 4:31 PM Jan Ekström <jeebjp@gmail.com> wrote:
>
> On Sat, Feb 13, 2021 at 12:48 AM sfan5 <sfan5@live.de> wrote:
> >
> > 12.02.21 - 21:43 - Andreas Rheinhardt:
> > > sfan5:
> > >> Hi,
> > >>
> > >>
> > >> attached v2 patch after discussion on IRC with JEEB (as he already
> > >> mentioned).
> > >>
> > >> Only change is that the log level turns to debug when missing parameter
> > >> sets are within spec (cf. 14496-15).
> > >>
> > >>
> > >> -        av_log(avctx, AV_LOG_ERROR, "Could not extract PPS/SPS from extradata");
> > >> -        ret = AVERROR_INVALIDDATA;
> > >> +        const int warn = is_avc && avctx->codec_tag != MKTAG('a','v','c','1') &&
> > >> +            avctx->codec_tag != MKTAG('a','v','c','2');
> > >> +        av_log(avctx, warn ? AV_LOG_WARNING : AV_LOG_DEBUG,
> > >> +            "Could not extract PPS/SPS from extradata\n");
> > >> +        ret = 0;
> > >>       }
> > > warn = is_avc && (avctx->codec_tag == MKTAG('a','v','c','1') ||
> > >                    avctx->codec_tag == MKTAG('a','v','c','2')
> > > is what you (should) want.
> > >
> > > - Andreas
> >
> > Thanks for pointing that out, you're correct.
> >
> > here's v3:
>
> After some brief discussion on IRC, this looks good to me :) .
>

Applied as 6f80953554b07635d3b52f76b03807d198a5e9d0 with the
indentation of the log message and the initial capital letter of the
commit message fixed.

Jan
diff mbox series

Patch

diff --git a/libavcodec/mediacodecdec.c b/libavcodec/mediacodecdec.c
index ac1725e466..f5d13b171e 100644
--- a/libavcodec/mediacodecdec.c
+++ b/libavcodec/mediacodecdec.c
@@ -167,8 +167,11 @@  static int h264_set_extradata(AVCodecContext *avctx, FFAMediaFormat *format)
         ff_AMediaFormat_setBuffer(format, "csd-1", (void*)data, data_size);
         av_freep(&data);
     } else {
-        av_log(avctx, AV_LOG_ERROR, "Could not extract PPS/SPS from extradata");
-        ret = AVERROR_INVALIDDATA;
+        const int warn = is_avc && (avctx->codec_tag == MKTAG('a','v','c','1') ||
+                                    avctx->codec_tag == MKTAG('a','v','c','2'));
+        av_log(avctx, warn ? AV_LOG_WARNING : AV_LOG_DEBUG,
+            "Could not extract PPS/SPS from extradata\n");
+        ret = 0;
     }
 
 done:
@@ -254,8 +257,10 @@  static int hevc_set_extradata(AVCodecContext *avctx, FFAMediaFormat *format)
 
         av_freep(&data);
     } else {
-        av_log(avctx, AV_LOG_ERROR, "Could not extract VPS/PPS/SPS from extradata");
-        ret = AVERROR_INVALIDDATA;
+        const int warn = is_nalff && avctx->codec_tag == MKTAG('h','v','c','1');
+        av_log(avctx, warn ? AV_LOG_WARNING : AV_LOG_DEBUG,
+            "Could not extract VPS/PPS/SPS from extradata\n");
+        ret = 0;
     }
 
 done: