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 | expand |
Context | Check | Description |
---|---|---|
andriy/configure | warning | Failed to apply patch |
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). [...]
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 --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");