Re: [FFmpeg-devel] [PATCH 1/2] Revert "avcodec/decode: Fix avcodec parameters when bsfs, are enable by decoder"

[Andreas Rheinhardt <andreas.rheinhardt@outlook.com>]

Zhao Zhili:
>> Otherwise the extradata used would be ISOBMFF if the input is
>> even though we use the *_mp4toannexb BSFs to convert it to
>> annex B to feed it to the actual decoder.
>>
>> (The mediacodec decoders also use said BSFs, yet they process
>> the extradata in a way that works even when using the ISOBMFF
>> extradata; in fact, using the converted extradata would break
>> their check for whether to warn for missing extradata for
>> the ISOBMFF without-in-band-header profiles.
>>
>> Furthermore, there are several users of the *_mp4toannexb BSFs
>> that don't ever touch extradata. They have not been touched.)
> 
> The solution is fragile and easy to be broken. It doesn’t look like a better
> solution than avcodec_parameters_to_context.
> 

What is fragile here? I consider avcodec_parameters_to_context() to be
fragile, as your solution involves maintaining a list of stuff that
needs to be checked for whether avcodec_parameters_to_context() should
be called (future BSFs may convert between different flavors of a codec
and need to change the codec tag accordingly (actually, the
*_mp4toannexb BSFs should reset the codec tag, so that it no longer says
"no in-band extradata"), yet changing the codec tag (except uppercasing
it) is not allowed for the user-facing AVCodecContext)).

Furthermore, discarding the info whether the extradata was ISOBMFF is
not good at all, as the mediacodec decoders show: It implicitly disables
the warning for missing extradata.

>>
>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
>> ---
>>  libavcodec/amfdec.c     |  8 ++++++--
>>  libavcodec/decode_bsf.h | 42 +++++++++++++++++++++++++++++++++++++++++
>>  libavcodec/rkmppdec.c   |  9 ++++++---
>>  3 files changed, 54 insertions(+), 5 deletions(-)
>>  create mode 100644 libavcodec/decode_bsf.h
>>
>> diff --git a/libavcodec/amfdec.c b/libavcodec/amfdec.c
>> index 44ccd63aaf..1840a1b3c0 100644
>> --- a/libavcodec/amfdec.c
>> +++ b/libavcodec/amfdec.c
>> @@ -25,6 +25,7 @@
>>  #include "libavutil/mem.h"
>>  #include "libavutil/time.h"
>>  #include "decode.h"
>> +#include "decode_bsf.h"
>>  #include "libavutil/mastering_display_metadata.h"
>>  
>>  #if CONFIG_D3D11VA
>> @@ -187,9 +188,12 @@ static int amf_init_decoder(AVCodecContext *avctx)
>>          AMF_ASSIGN_PROPERTY_INT64(res, ctx->decoder, AMF_VIDEO_DECODER_SURFACE_COPY, ctx->copy_output);
>>  
>>      if (avctx->extradata_size) {
>> -        res = amf_device_ctx->context->pVtbl->AllocBuffer(amf_device_ctx->context, AMF_MEMORY_HOST, avctx->extradata_size, &buffer);
>> +        const uint8_t *extradata;
>> +        int extradata_size;
>> +        ff_decode_get_extradata(avctx, &extradata, &extradata_size);
>> +        res = amf_device_ctx->context->pVtbl->AllocBuffer(amf_device_ctx->context, AMF_MEMORY_HOST, extradata_size, &buffer);
>>          if (res == AMF_OK) {
>> -            memcpy(buffer->pVtbl->GetNative(buffer), avctx->extradata, avctx->extradata_size);
>> +            memcpy(buffer->pVtbl->GetNative(buffer), extradata, extradata_size);
>>              AMF_ASSIGN_PROPERTY_INTERFACE(res,ctx->decoder, AMF_VIDEO_DECODER_EXTRADATA, buffer);
>>              buffer->pVtbl->Release(buffer);
>>              buffer = NULL;
>> diff --git a/libavcodec/decode_bsf.h b/libavcodec/decode_bsf.h
>> new file mode 100644
>> index 0000000000..9ea9ab70c1
>> --- /dev/null
>> +++ b/libavcodec/decode_bsf.h
>> @@ -0,0 +1,42 @@
>> +/*
>> + * This file is part of FFmpeg.
>> + *
>> + * FFmpeg is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU Lesser General Public
>> + * License as published by the Free Software Foundation; either
>> + * version 2.1 of the License, or (at your option) any later version.
>> + *
>> + * FFmpeg is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>> + * Lesser General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU Lesser General Public
>> + * License along with FFmpeg; if not, write to the Free Software
>> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
>> + */
>> +
>> +#ifndef AVCODEC_DECODE_BSF_H
>> +#define AVCODEC_DECODE_BSF_H
>> +
>> +#include <stdint.h>
>> +
>> +#include "avcodec.h"
>> +#include "bsf.h"
>> +#include "internal.h"
>> +
>> +/**
>> + * Helper function for decoders that may use a BSF that changes extradata.
>> + * This function will get the extradata from the BSF.
>> + */
>> +static inline void ff_decode_get_extradata(const AVCodecContext *avctx,
>> +                                           const uint8_t **extradata,
>> +                                           int *extradata_size)
>> +{
>> +    // Given that we unconditionally insert a null BSF when no BSF is
>> +    // explicitly requested, we can just use the BSF's par_out here.
>> +    *extradata      = avctx->internal->bsf->par_out->extradata;
>> +    *extradata_size = avctx->internal->bsf->par_out->extradata_size;
>> +}
>> +
>> +#endif /* AVCODEC_DECODE_BSF_H */
>> diff --git a/libavcodec/rkmppdec.c b/libavcodec/rkmppdec.c
>> index 54e697664e..dad0e4c25f 100644
>> --- a/libavcodec/rkmppdec.c
>> +++ b/libavcodec/rkmppdec.c
>> @@ -29,6 +29,7 @@
>>  #include "avcodec.h"
>>  #include "codec_internal.h"
>>  #include "decode.h"
>> +#include "decode_bsf.h"
>>  #include "hwconfig.h"
>>  #include "libavutil/refstruct.h"
>>  #include "libavutil/buffer.h"
>> @@ -279,9 +280,11 @@ static int rkmpp_send_packet(AVCodecContext *avctx, const AVPacket *avpkt)
>>      // on first packet, send extradata
>>      if (decoder->first_packet) {
>>          if (avctx->extradata_size) {
>> -            ret = rkmpp_write_data(avctx, avctx->extradata,
>> -                                            avctx->extradata_size,
>> -                                            avpkt->pts);
>> +            const uint8_t *extradata;
>> +            int extradata_size;
>> +            ff_decode_get_extradata(avctx, &extradata, &extradata_size);
>> +            ret = rkmpp_write_data(avctx, (uint8_t*)extradata, extradata_size,
>> +                                   avpkt->pts);
>>              if (ret) {
>>                  av_log(avctx, AV_LOG_ERROR, "Failed to write extradata to decoder (code = %d)\n", ret);
>>                  return ret;
>> -- 
>> 2.45.2
>>
_______________________________________________
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".