diff mbox series

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

Message ID DB6PR0901MB149581C79FF0CF3961494EB8ECBD0@DB6PR0901MB1495.eurprd09.prod.outlook.com
State Superseded
Headers show
Series [FFmpeg-devel] avcodec/mediacodecdec: Do not abort when H264/HEVC extradata extraction fails
Related show

Checks

Context Check Description
andriy/configure warning Failed to apply patch
andriy/configure warning Failed to apply patch

Commit Message

sfan5 Jan. 25, 2021, 5:38 p.m. UTC
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 | 8 ++++----
  1 file changed, 4 insertions(+), 4 deletions(-)

+        ret = 0;
      }
   done:
@@ -254,8 +254,8 @@ 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;
+        av_log(avctx, AV_LOG_WARNING, "Could not extract VPS/PPS/SPS 
from extradata\n");
+        ret = 0;
      }
   done:

Comments

Matthieu Bouron Feb. 12, 2021, 10:07 a.m. UTC | #1
On Mon, Jan 25, 2021 at 06:38:36PM +0100, sfan5 wrote:
> 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 | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/libavcodec/mediacodecdec.c b/libavcodec/mediacodecdec.c
> index ac1725e466..67adefb530 100644
> --- a/libavcodec/mediacodecdec.c
> +++ b/libavcodec/mediacodecdec.c
> @@ -167,8 +167,8 @@ 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;
> +        av_log(avctx, AV_LOG_WARNING, "Could not extract PPS/SPS from
> extradata\n");
> +        ret = 0;
>      }
>   done:
> @@ -254,8 +254,8 @@ 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;
> +        av_log(avctx, AV_LOG_WARNING, "Could not extract VPS/PPS/SPS from
> extradata\n");
> +        ret = 0;
>      }
>   done:

Hello,

First of all, sorry for the late reply. The original intent with this
restriction was to make sure that MediaCodec could error out early (at the
configuration stage) so an implementation on top of libavcodec can decide
to fall back to a software decoder (without trying to decode anything).

I am fine with removing this restriction (if everyone is OK with that) for
the following reasons:
- there are chances that the h264/hevc software decoders have not been
  enabled (to avoid licensing issues) so the fallback mechanism is not
  available
- as you said extradata are not always available (mpegts) and we want to
  give MediaCodec a chance to decode the stream

So LGTM.

If everyone is OK with that, I'll also submit a patch that removes the
requirements on having the avctx->extradata set (I use such patch at work
to allow playback of certain hls/mpegts streams).

[...]
Jan Ekström Feb. 12, 2021, 4:54 p.m. UTC | #2
On Fri, Feb 12, 2021 at 12:36 PM Matthieu Bouron
<matthieu.bouron@gmail.com> wrote:
>
> On Mon, Jan 25, 2021 at 06:38:36PM +0100, sfan5 wrote:
> > 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 | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/libavcodec/mediacodecdec.c b/libavcodec/mediacodecdec.c
> > index ac1725e466..67adefb530 100644
> > --- a/libavcodec/mediacodecdec.c
> > +++ b/libavcodec/mediacodecdec.c
> > @@ -167,8 +167,8 @@ 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;
> > +        av_log(avctx, AV_LOG_WARNING, "Could not extract PPS/SPS from
> > extradata\n");
> > +        ret = 0;
> >      }
> >   done:
> > @@ -254,8 +254,8 @@ 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;
> > +        av_log(avctx, AV_LOG_WARNING, "Could not extract VPS/PPS/SPS from
> > extradata\n");
> > +        ret = 0;
> >      }
> >   done:
>
> Hello,
>
> First of all, sorry for the late reply. The original intent with this
> restriction was to make sure that MediaCodec could error out early (at the
> configuration stage) so an implementation on top of libavcodec can decide
> to fall back to a software decoder (without trying to decode anything).
>
> I am fine with removing this restriction (if everyone is OK with that) for
> the following reasons:
> - there are chances that the h264/hevc software decoders have not been
>   enabled (to avoid licensing issues) so the fallback mechanism is not
>   available
> - as you said extradata are not always available (mpegts) and we want to
>   give MediaCodec a chance to decode the stream

I just had a discussion with sfan5 on IRC yesterday when I was looking
at the related code, and yea - I think this is acceptable since to get
to this point ff_h264_decode_extradata / ff_hevc_decode_extradata
would have to return success. They just didn't get anything. This can
happen with f.ex. completely valid MP4 files, since certain
identifiers (which are visible through the codec_tag identifier) do
not required all of the required parameter sets to be within the
extradata (and thus having 0 pps and 0 sps is technically valid with
those).

My idea was thus to make the log message either WARNING or DEBUG
depending on (is_avc || is_nalff) && codec_tag in (list, of, tags) (in
pseudocode). How matroska should be handled depends on how the codec
stuff is currently defined in there.

>
> So LGTM.
>
> If everyone is OK with that, I'll also submit a patch that removes the
> requirements on having the avctx->extradata set (I use such patch at work
> to allow playback of certain hls/mpegts streams).
>
> [...]
> --
> Matthieu B.
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
diff mbox series

Patch

diff --git a/libavcodec/mediacodecdec.c b/libavcodec/mediacodecdec.c
index ac1725e466..67adefb530 100644
--- a/libavcodec/mediacodecdec.c
+++ b/libavcodec/mediacodecdec.c
@@ -167,8 +167,8 @@  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;
+        av_log(avctx, AV_LOG_WARNING, "Could not extract PPS/SPS from 
extradata\n");