[FFmpeg-devel] [PATCH] avcodec/cuviddec: fix AV1 decoding error

[FFmpeg-devel] [PATCH] avcodec/cuviddec: fix AV1 decoding error

[Zhao Zhili]

From: Zhao Zhili <zhilizhao@tencent.com>

Regression from 45e3b6a68. cuvidParseVideoData report unknown
error with AV1CodecConfigurationRecord.
---
 libavcodec/cuviddec.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/libavcodec/cuviddec.c b/libavcodec/cuviddec.c
index cb3cda7e24..584fff40ed 100644
--- a/libavcodec/cuviddec.c
+++ b/libavcodec/cuviddec.c
@@ -953,6 +953,17 @@ static av_cold int cuvid_decode_init(AVCodecContext *avctx)
         extradata_size = avctx->extradata_size;
     }
 
+#if CONFIG_AV1_CUVID_DECODER && defined(CUVID_HAS_AV1_SUPPORT)
+    if (avctx->codec->id == AV_CODEC_ID_AV1 &&
+            extradata_size > 4 &&
+            extradata && extradata[0] & 0x80) {
+        // skip AV1CodecConfigurationRecord to keep configOBUs only,
+        // otherwise cuvidParseVideoData report unknown error.
+        extradata += 4;
+        extradata_size -= 4;
+    }
+#endif
+
     ctx->cuparse_ext = av_mallocz(sizeof(*ctx->cuparse_ext)
             + FFMAX(extradata_size - (int)sizeof(ctx->cuparse_ext->raw_seqhdr_data), 0));
     if (!ctx->cuparse_ext) {
-- 
2.35.3

_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH] avcodec/cuviddec: fix AV1 decoding error

[Timo Rothenpieler]

On 23/06/2022 11:10, Zhao Zhili wrote:
> From: Zhao Zhili <zhilizhao@tencent.com>
> 
> Regression from 45e3b6a68. cuvidParseVideoData report unknown
> error with AV1CodecConfigurationRecord.
> ---
>   libavcodec/cuviddec.c | 11 +++++++++++
>   1 file changed, 11 insertions(+)
> 
> diff --git a/libavcodec/cuviddec.c b/libavcodec/cuviddec.c
> index cb3cda7e24..584fff40ed 100644
> --- a/libavcodec/cuviddec.c
> +++ b/libavcodec/cuviddec.c
> @@ -953,6 +953,17 @@ static av_cold int cuvid_decode_init(AVCodecContext *avctx)
>           extradata_size = avctx->extradata_size;
>       }
>   
> +#if CONFIG_AV1_CUVID_DECODER && defined(CUVID_HAS_AV1_SUPPORT)
> +    if (avctx->codec->id == AV_CODEC_ID_AV1 &&
> +            extradata_size > 4 &&
> +            extradata && extradata[0] & 0x80) {
> +        // skip AV1CodecConfigurationRecord to keep configOBUs only,
> +        // otherwise cuvidParseVideoData report unknown error.
> +        extradata += 4;
> +        extradata_size -= 4;
> +    }
> +#endif

Checking the defines shouldn't be necessary. The codec ID should never 
be AV1 if it's not enabled, and the code does not contain anything AV1 
specific.

>       ctx->cuparse_ext = av_mallocz(sizeof(*ctx->cuparse_ext)
>               + FFMAX(extradata_size - (int)sizeof(ctx->cuparse_ext->raw_seqhdr_data), 0));
>       if (!ctx->cuparse_ext) {

I'm not quite sure I understand what causes this.
Something about the mp4 demuxer changed, yes.
But what about other containers? Why wasn't this an issue before?

The code that was removed in 45e3b6a68 seems to just have always skipped 
the first 4 byte unconditionally.

A little more info in the commit message would be nice here. Specially a 
sentence or two about the "& 80" part.
Code LGTM otherwise.
_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH] avcodec/cuviddec: fix AV1 decoding error

["zhilizhao(赵志立)"]

> On Jun 23, 2022, at 10:14 PM, Timo Rothenpieler <timo@rothenpieler.org> wrote:
> 
> On 23/06/2022 11:10, Zhao Zhili wrote:
>> From: Zhao Zhili <zhilizhao@tencent.com>
>> Regression from 45e3b6a68. cuvidParseVideoData report unknown
>> error with AV1CodecConfigurationRecord.
>> ---
>>  libavcodec/cuviddec.c | 11 +++++++++++
>>  1 file changed, 11 insertions(+)
>> diff --git a/libavcodec/cuviddec.c b/libavcodec/cuviddec.c
>> index cb3cda7e24..584fff40ed 100644
>> --- a/libavcodec/cuviddec.c
>> +++ b/libavcodec/cuviddec.c
>> @@ -953,6 +953,17 @@ static av_cold int cuvid_decode_init(AVCodecContext *avctx)
>>          extradata_size = avctx->extradata_size;
>>      }
>>  +#if CONFIG_AV1_CUVID_DECODER && defined(CUVID_HAS_AV1_SUPPORT)
>> +    if (avctx->codec->id == AV_CODEC_ID_AV1 &&
>> +            extradata_size > 4 &&
>> +            extradata && extradata[0] & 0x80) {
>> +        // skip AV1CodecConfigurationRecord to keep configOBUs only,
>> +        // otherwise cuvidParseVideoData report unknown error.
>> +        extradata += 4;
>> +        extradata_size -= 4;
>> +    }
>> +#endif
> 
> Checking the defines shouldn't be necessary. The codec ID should never be AV1 if it's not enabled, and the code does not contain anything AV1 specific.

I have thought about this. On one hand, the conditional compile is
unnecessary. On the other hand, it can remove dead code explicitly
when the condition is false, instead of depending on compiler.

I’m OK with remove the condition to keep code simple.

> 
>>      ctx->cuparse_ext = av_mallocz(sizeof(*ctx->cuparse_ext)
>>              + FFMAX(extradata_size - (int)sizeof(ctx->cuparse_ext->raw_seqhdr_data), 0));
>>      if (!ctx->cuparse_ext) {
> 
> I'm not quite sure I understand what causes this.
> Something about the mp4 demuxer changed, yes.
> But what about other containers? Why wasn't this an issue before?

It’s a regression from the point of fftools. If we take libavcodec
as standalone lib, the issue exist before 45e3b6a68.

> 
> The code that was removed in 45e3b6a68 seems to just have always skipped the first 4 byte unconditionally.

In the context of mp4 demuxer, it’s guaranteed the extradata is
AV1CodecConfigurationRecord. There is no guarantee that the
extradata is AV1CodecConfigurationRecord in the context of
decoder. So that’s why the check `extradata[0] & 0x80` exist.

AV1CodecConfigurationRecord is used by mp4 and mkv both. And
there is a similar patch in mkv: ffd1316

    avformat/matroskadec: remove special handling of av1c extradata

    Libavcodec can now handle the standard AV1CodecConfigurationRecord
    extradata as-is.

> 
> A little more info in the commit message would be nice here. Specially a sentence or two about the "& 80" part.

Will add comments about the “& 80” part.

Thanks for the review!

> Code LGTM otherwise.
> _______________________________________________
> 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".

_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH] avcodec/cuviddec: fix AV1 decoding error

["zhilizhao(赵志立)"]

> On Jun 24, 2022, at 10:52 AM, zhilizhao(赵志立) <quinkblack@foxmail.com> wrote:
> 
> 
> 
>> On Jun 23, 2022, at 10:14 PM, Timo Rothenpieler <timo@rothenpieler.org> wrote:
>> 
>> On 23/06/2022 11:10, Zhao Zhili wrote:
>>> From: Zhao Zhili <zhilizhao@tencent.com>
>>> Regression from 45e3b6a68. cuvidParseVideoData report unknown
>>> error with AV1CodecConfigurationRecord.
>>> ---
>>> libavcodec/cuviddec.c | 11 +++++++++++
>>> 1 file changed, 11 insertions(+)
>>> diff --git a/libavcodec/cuviddec.c b/libavcodec/cuviddec.c
>>> index cb3cda7e24..584fff40ed 100644
>>> --- a/libavcodec/cuviddec.c
>>> +++ b/libavcodec/cuviddec.c
>>> @@ -953,6 +953,17 @@ static av_cold int cuvid_decode_init(AVCodecContext *avctx)
>>>         extradata_size = avctx->extradata_size;
>>>     }
>>> +#if CONFIG_AV1_CUVID_DECODER && defined(CUVID_HAS_AV1_SUPPORT)
>>> +    if (avctx->codec->id == AV_CODEC_ID_AV1 &&
>>> +            extradata_size > 4 &&
>>> +            extradata && extradata[0] & 0x80) {
>>> +        // skip AV1CodecConfigurationRecord to keep configOBUs only,
>>> +        // otherwise cuvidParseVideoData report unknown error.
>>> +        extradata += 4;
>>> +        extradata_size -= 4;
>>> +    }
>>> +#endif
>> 
...
> 
>> 
>> A little more info in the commit message would be nice here. Specially a sentence or two about the "& 80" part.
> 
> Will add comments about the “& 80” part.

v2 (Sorry I forgot to add v2 in patch subject):

http://ffmpeg.org/pipermail/ffmpeg-devel/2022-June/297983.html

> 
> Thanks for the review!
> 
>> Code LGTM otherwise.
>> _______________________________________________
>> 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".
> 
> _______________________________________________
> 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".

_______________________________________________
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".

[FFmpeg-devel] [PATCH] avcodec/cuviddec: fix AV1 decoding error

[Zhao Zhili]

From: Zhao Zhili <zhilizhao@tencent.com>

cuvidParseVideoData only supports pure OBUs, it report unknown
error with AV1CodecConfigurationRecord. Check whether extradata
is AV1CodecConfigurationRecord and skip the first 4 bytes to fix
the issue.

The bug is revealed in ffmpeg cmd since 45e3b6a68 and ffd1316e.
---
 libavcodec/cuviddec.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/libavcodec/cuviddec.c b/libavcodec/cuviddec.c
index cb3cda7e24..fc9c0515fe 100644
--- a/libavcodec/cuviddec.c
+++ b/libavcodec/cuviddec.c
@@ -953,6 +953,16 @@ static av_cold int cuvid_decode_init(AVCodecContext *avctx)
         extradata_size = avctx->extradata_size;
     }
 
+    // Check first bit to determine whether it's AV1CodecConfigurationRecord.
+    // Skip first 4 bytes of AV1CodecConfigurationRecord to keep configOBUs
+    // only, otherwise cuvidParseVideoData report unknown error.
+    if (avctx->codec->id == AV_CODEC_ID_AV1 &&
+            extradata_size > 4 &&
+            extradata && extradata[0] & 0x80) {
+        extradata += 4;
+        extradata_size -= 4;
+    }
+
     ctx->cuparse_ext = av_mallocz(sizeof(*ctx->cuparse_ext)
             + FFMAX(extradata_size - (int)sizeof(ctx->cuparse_ext->raw_seqhdr_data), 0));
     if (!ctx->cuparse_ext) {
-- 
2.34.1

_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH] avcodec/cuviddec: fix AV1 decoding error

[James Almer]

On 6/24/2022 1:04 AM, Zhao Zhili wrote:
> From: Zhao Zhili <zhilizhao@tencent.com>
> 
> cuvidParseVideoData only supports pure OBUs, it report unknown
> error with AV1CodecConfigurationRecord. Check whether extradata
> is AV1CodecConfigurationRecord and skip the first 4 bytes to fix
> the issue.
> 
> The bug is revealed in ffmpeg cmd since 45e3b6a68 and ffd1316e.
> ---
>   libavcodec/cuviddec.c | 10 ++++++++++
>   1 file changed, 10 insertions(+)
> 
> diff --git a/libavcodec/cuviddec.c b/libavcodec/cuviddec.c
> index cb3cda7e24..fc9c0515fe 100644
> --- a/libavcodec/cuviddec.c
> +++ b/libavcodec/cuviddec.c
> @@ -953,6 +953,16 @@ static av_cold int cuvid_decode_init(AVCodecContext *avctx)
>           extradata_size = avctx->extradata_size;
>       }
>   
> +    // Check first bit to determine whether it's AV1CodecConfigurationRecord.
> +    // Skip first 4 bytes of AV1CodecConfigurationRecord to keep configOBUs
> +    // only, otherwise cuvidParseVideoData report unknown error.
> +    if (avctx->codec->id == AV_CODEC_ID_AV1 &&
> +            extradata_size > 4 &&
> +            extradata && extradata[0] & 0x80) {

IMO the check for extradata here is unnecessary as the memcpy 
dereferencing it below will crash anyway if it were NULL.

If anything, a check like (avctx->extradata_size || !avctx->extradata) 
should be added to avcodec_open2() if there isn't one already to ensure 
the user set up valid extradata when opening a decoder.

> +        extradata += 4;
> +        extradata_size -= 4;
> +    }
> +
>       ctx->cuparse_ext = av_mallocz(sizeof(*ctx->cuparse_ext)
>               + FFMAX(extradata_size - (int)sizeof(ctx->cuparse_ext->raw_seqhdr_data), 0));
>       if (!ctx->cuparse_ext) {

LGTM either way.
_______________________________________________
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".