[FFmpeg-devel] avformat/mxfenc: SMPTE RDD 48:2018 Amd 1:2022 (FFV1 in MXF) support

[Jerome Martinez]

On 01/04/2023 17:43, Michael Niedermayer wrote:
> On Sat, Apr 01, 2023 at 05:20:50PM +0200, Jerome Martinez wrote:
>> On 01/04/2023 16:37, Michael Niedermayer wrote:
>>> On Tue, Mar 14, 2023 at 10:52:15AM +0100, Jerome Martinez wrote:
>>> [...]
>>>> +
>>>> +    memset(state, 128, sizeof(state));
>>>> +    if (st->codecpar->extradata) {
>>>> +        ff_init_range_decoder(&c, st->codecpar->extradata, st->codecpar->extradata_size);
>>>> +        ff_build_rac_states(&c, 0.05 * (1LL << 32), 256 - 8);
>>>> +        v = get_ffv1_unsigned_symbol(&c, state);
>>>> +        av_assert0(v >= 2);
>>>> +        if (v > 4) {
>>>> +            return 0;
>>>> +        }
>>>> +        sc->micro_version = get_ffv1_unsigned_symbol(&c, state);
>>>> +    } else {
>>>> +        uint8_t keystate = 128;
>>>> +        ff_init_range_decoder(&c, pkt->data, pkt->size);
>>>> +        ff_build_rac_states(&c, 0.05 * (1LL << 32), 256 - 8);
>>>> +        get_rac(&c, &keystate); // keyframe
>>>> +        v = get_ffv1_unsigned_symbol(&c, state);
>>>> +        av_assert0(v < 2);
>>> Are these asserts testing muxer input ?
>>> if so what ensures that the values are within the asserted range ?
>>
>> My understanding of the code and workflow is that the input is currently
>> rejected (AV_LOG_ERROR, "invalid version %d in ver01 header\n" and
>> AV_LOG_ERROR, "Invalid version in global header\n") in ffv1dec.c during the
>> analysis of this input so before this piece of code is reached.
>> Could be an AV_LOG_ERROR if preferred.
> if the encoder writes 5 teh muxer crashes here
>
> V 5me=    0 fps=0.0 q=-0.0 size=       0kB time=00:00:00.00 bitrate=N/A speed=   0x
> Assertion v < 2 failed at libavformat/mxfenc.c:2505


I hardcoded version 5 in the encoder based on version 4 (so with extra 
metadata), and I have:
[mxf @ 0x7fffd52f3100] could not get ffv1 version

My understanding is that you hardcoded version 5 in the encoder based on 
version 0 or 1 (so without extra metadata), in that case I have the 
assert, and it was on purpose on my side because I understand from ffv1 
encoder code that any version > 2 should have extra metadata, and if it 
changes in the future this line in MXF muxer should be changed at the 
same time as FFV1 encoder code

But I didn't test and didn't realize that the FFV1 parsing is not done 
with -c:v copy, and if that case I get the assert with an (fake) input 
with version 5 and no extra metadata.

What about the attached patch? I reused the ffv1 decoder error messages 
+ "ffv1 " tip.
Note: I moved, so less coherency in the code, the error message in 
mxf_parse_ffv1_frame for more precise message depending on the presence 
or not of extra metadata, it could stay as is if we don't care about the 
precision.
-------------- next part --------------
From 65f6fba9434fa9ab4b5d2b91c3f930c1e3cb4bef Mon Sep 17 00:00:00 2001
From: Jerome Martinez <jerome at mediaarea.net>
Date: Sat, 1 Apr 2023 19:04:25 +0200
Subject: [PATCH] avformat/mxfenc: replace ffv1 version related assert by error
 message

---
 libavformat/mxfenc.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/libavformat/mxfenc.c b/libavformat/mxfenc.c
index 9eba208ffb..0a1ae5353c 100644
--- a/libavformat/mxfenc.c
+++ b/libavformat/mxfenc.c
@@ -2489,8 +2489,8 @@ static int mxf_parse_ffv1_frame(AVFormatContext *s, AVStream *st, AVPacket *pkt)
         ff_init_range_decoder(&c, st->codecpar->extradata, st->codecpar->extradata_size);
         ff_build_rac_states(&c, 0.05 * (1LL << 32), 256 - 8);
         v = get_ffv1_unsigned_symbol(&c, state);
-        av_assert0(v >= 2);
-        if (v > 4) {
+        if (v < 2) {
+            av_log(s, AV_LOG_ERROR, "Invalid version in ffv1 global header\n");
             return 0;
         }
         sc->micro_version = get_ffv1_unsigned_symbol(&c, state);
@@ -2500,7 +2500,10 @@ static int mxf_parse_ffv1_frame(AVFormatContext *s, AVStream *st, AVPacket *pkt)
         ff_build_rac_states(&c, 0.05 * (1LL << 32), 256 - 8);
         get_rac(&c, &keystate); // keyframe
         v = get_ffv1_unsigned_symbol(&c, state);
-        av_assert0(v < 2);
+        if (v >= 2) {
+            av_log(s, AV_LOG_ERROR, "invalid version %d in ffv1 ver01 header\n", v);
+            return 0;
+        }
     }
     sc->codec_ul = &mxf_ffv1_codec_uls[v];
 
@@ -3129,7 +3132,6 @@ static int mxf_write_packet(AVFormatContext *s, AVPacket *pkt)
         }
     } else if (st->codecpar->codec_id == AV_CODEC_ID_FFV1) {
         if (!mxf_parse_ffv1_frame(s, st, pkt)) {
-            av_log(s, AV_LOG_ERROR, "could not get ffv1 version\n");
             return -1;
         }
     }
-- 
2.13.3.windows.1