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

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

[Jerome Martinez]

[-- Attachment #1: Type: text/plain, Size: 791 bytes --]

The arbitrary short element codes are the ones used by another muxer ( 
files available at 
https://www.digitizationguidelines.gov/guidelines/MXF_app_sampleFiles.html#2022 
)

The support of RGBA descriptor is added, mainly by disabling in the CDCI 
descriptor related code the elements not in the Generic picture 
descriptor, and could be in a separated dedicated patch (move of Generic 
picture descriptor code in a dedicated function?).

Tested with:
ffmpeg -f lavfi -i testsrc=duration=10:size=ntsc:rate=ntsc -field_order 
bb -c:v ffv1 -level 0 test_ffv1_ntsc.mxf
ffmpeg -f lavfi -i testsrc=duration=10:size=pal:rate=pal -field_order tt 
-c:v ffv1 -level 3 test_ffv1_pal.mxf
ffmpeg -f lavfi -i testsrc=duration=10:size=1920x1080 -pix_fmt yuv422p10 
-c:v ffv1 -level 3 test_ffv1_hd.mxf



[-- Attachment #2: 0001-avformat-mxfenc-SMPTE-RDD-48-2018-Amd-1-2022-support.patch --]
[-- Type: text/plain, Size: 14544 bytes --]

From 2e38dc0a114a1706f6d108ea9ca3e86083bfc2eb Mon Sep 17 00:00:00 2001
From: Jerome Martinez <jerome@mediaarea.net>
Date: Wed, 4 Jan 2023 13:56:21 +0100
Subject: [PATCH] avformat/mxfenc: SMPTE RDD 48:2018 Amd 1:2022 support

---
 libavformat/Makefile         |   3 +-
 libavformat/mxfenc.c         | 163 ++++++++++++++++++++++++++++++++++++++++++-
 libavformat/rangecoder_dec.c |   1 +
 3 files changed, 164 insertions(+), 3 deletions(-)
 create mode 100644 libavformat/rangecoder_dec.c

diff --git a/libavformat/Makefile b/libavformat/Makefile
index fa71ec12f7..3f80acbed9 100644
--- a/libavformat/Makefile
+++ b/libavformat/Makefile
@@ -707,7 +707,8 @@ SHLIBOBJS-$(CONFIG_HLS_DEMUXER)          += ac3_channel_layout_tab.o
 SHLIBOBJS-$(CONFIG_MATROSKA_DEMUXER)     += mpeg4audio_sample_rates.o
 SHLIBOBJS-$(CONFIG_MOV_DEMUXER)          += ac3_channel_layout_tab.o
 SHLIBOBJS-$(CONFIG_MP3_MUXER)            += mpegaudiotabs.o
-SHLIBOBJS-$(CONFIG_MXF_MUXER)            += golomb_tab.o
+SHLIBOBJS-$(CONFIG_MXF_MUXER)            += golomb_tab.o \
+                                            rangecoder_dec.o
 SHLIBOBJS-$(CONFIG_NUT_MUXER)            += mpegaudiotabs.o
 SHLIBOBJS-$(CONFIG_RTPDEC)               += jpegtables.o
 SHLIBOBJS-$(CONFIG_RTP_MUXER)            += golomb_tab.o jpegtables.o \
diff --git a/libavformat/mxfenc.c b/libavformat/mxfenc.c
index 58c551c83c..206b3484aa 100644
--- a/libavformat/mxfenc.c
+++ b/libavformat/mxfenc.c
@@ -51,6 +51,7 @@
 #include "libavcodec/golomb.h"
 #include "libavcodec/h264.h"
 #include "libavcodec/packet_internal.h"
+#include "libavcodec/rangecoder.h"
 #include "libavcodec/startcode.h"
 #include "avformat.h"
 #include "avio_internal.h"
@@ -99,6 +100,7 @@ typedef struct MXFStreamContext {
     int b_picture_count;     ///< maximum number of consecutive b pictures, used in mpeg-2 descriptor
     int low_delay;           ///< low delay, used in mpeg-2 descriptor
     int avc_intra;
+    int micro_version;       ///< format micro_version, used in ffv1 descriptor
 } MXFStreamContext;
 
 typedef struct MXFContainerEssenceEntry {
@@ -127,6 +129,7 @@ enum ULIndex {
     INDEX_H264,
     INDEX_S436M,
     INDEX_PRORES,
+    INDEX_FFV1,
 };
 
 static const struct {
@@ -141,6 +144,7 @@ static const struct {
     { AV_CODEC_ID_JPEG2000,   INDEX_JPEG2000 },
     { AV_CODEC_ID_H264,       INDEX_H264 },
     { AV_CODEC_ID_PRORES,     INDEX_PRORES },
+    { AV_CODEC_ID_FFV1,       INDEX_FFV1 },
     { AV_CODEC_ID_NONE }
 };
 
@@ -148,6 +152,7 @@ static void mxf_write_wav_desc(AVFormatContext *s, AVStream *st);
 static void mxf_write_aes3_desc(AVFormatContext *s, AVStream *st);
 static void mxf_write_mpegvideo_desc(AVFormatContext *s, AVStream *st);
 static void mxf_write_h264_desc(AVFormatContext *s, AVStream *st);
+static void mxf_write_ffv1_desc(AVFormatContext *s, AVStream *st);
 static void mxf_write_cdci_desc(AVFormatContext *s, AVStream *st);
 static void mxf_write_generic_sound_desc(AVFormatContext *s, AVStream *st);
 static void mxf_write_s436m_anc_desc(AVFormatContext *s, AVStream *st);
@@ -205,6 +210,11 @@ static const MXFContainerEssenceEntry mxf_essence_container_uls[] = {
       { 0x06,0x0E,0x2B,0x34,0x01,0x02,0x01,0x01,0x0d,0x01,0x03,0x01,0x15,0x01,0x17,0x00 },
       { 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x0d,0x04,0x01,0x02,0x02,0x03,0x06,0x03,0x00 },
       mxf_write_cdci_desc },
+    // FFV1
+    { { 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x0d,0x0d,0x01,0x03,0x01,0x02,0x23,0x01,0x00 },
+      { 0x06,0x0E,0x2B,0x34,0x01,0x02,0x01,0x01,0x0d,0x01,0x03,0x01,0x15,0x01,0x1d,0x00 },
+      { 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x0d,0x04,0x01,0x02,0x02,0x03,0x09,0x00,0x00 },
+      mxf_write_ffv1_desc },
     { { 0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00 },
       { 0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00 },
       { 0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00 },
@@ -229,6 +239,15 @@ static const UID mxf_d10_container_uls[] = {
     { 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x01,0x0D,0x01,0x03,0x01,0x02,0x01,0x06,0x01 }, // D-10 525/50 NTSC 30mb/s
 };
 
+
+static const UID mxf_ffv1_codec_uls[] = {
+    { 0x06,0x0e,0x2b,0x34,0x04,0x01,0x01,0x0D,0x04,0x01,0x02,0x02,0x03,0x09,0x01,0x00 }, // FFV1 version 0
+    { 0x06,0x0e,0x2b,0x34,0x04,0x01,0x01,0x0D,0x04,0x01,0x02,0x02,0x03,0x09,0x02,0x00 }, // FFV1 version 1
+    { 0x06,0x0e,0x2b,0x34,0x04,0x01,0x01,0x0D,0x04,0x01,0x02,0x02,0x03,0x09,0x03,0x00 }, // FFV1 version 2 (was only experimental)
+    { 0x06,0x0e,0x2b,0x34,0x04,0x01,0x01,0x0D,0x04,0x01,0x02,0x02,0x03,0x09,0x04,0x00 }, // FFV1 version 3
+    { 0x06,0x0e,0x2b,0x34,0x04,0x01,0x01,0x0D,0x04,0x01,0x02,0x02,0x03,0x09,0x05,0x00 }, // FFV1 version 4
+};
+
 static const uint8_t product_uid[]          = { 0xAD,0xAB,0x44,0x24,0x2f,0x25,0x4d,0xc7,0x92,0xff,0x29,0xbd,0x00,0x0c,0x00,0x02};
 static const uint8_t uuid_base[]            = { 0xAD,0xAB,0x44,0x24,0x2f,0x25,0x4d,0xc7,0x92,0xff };
 static const uint8_t umid_ul[]              = { 0x06,0x0A,0x2B,0x34,0x01,0x01,0x01,0x05,0x01,0x01,0x0D,0x00,0x13 };
@@ -387,6 +406,10 @@ static const MXFLocalTagPair mxf_local_tag_batch[] = {
     { 0x8302, FF_MXF_MasteringDisplayWhitePointChromaticity },
     { 0x8303, FF_MXF_MasteringDisplayMaximumLuminance },
     { 0x8304, FF_MXF_MasteringDisplayMinimumLuminance },
+    // FFV1
+    { 0xDFD9, {0x06,0x0E,0x2B,0x34,0x01,0x01,0x01,0x0E,0x04,0x01,0x06,0x0C,0x06,0x00,0x00,0x00}}, /* FFV1 Micro-version */
+    { 0xDFDA, {0x06,0x0E,0x2B,0x34,0x01,0x01,0x01,0x0E,0x04,0x01,0x06,0x0C,0x05,0x00,0x00,0x00}}, /* FFV1 Version */
+    { 0xDFDB, {0x06,0x0E,0x2B,0x34,0x01,0x01,0x01,0x0E,0x04,0x01,0x06,0x0C,0x01,0x00,0x00,0x00}}, /* FFV1 Initialization Metadata */
 };
 
 #define MXF_NUM_TAGS FF_ARRAY_ELEMS(mxf_local_tag_batch)
@@ -523,7 +546,7 @@ static void mxf_write_primer_pack(AVFormatContext *s)
     MXFContext *mxf = s->priv_data;
     AVIOContext *pb = s->pb;
     int local_tag_number = MXF_NUM_TAGS, i;
-    int will_have_avc_tags = 0, will_have_mastering_tags = 0;
+    int will_have_avc_tags = 0, will_have_mastering_tags = 0, will_have_ffv1_tags = 0;
 
     for (i = 0; i < s->nb_streams; i++) {
         MXFStreamContext *sc = s->streams[i]->priv_data;
@@ -533,6 +556,9 @@ static void mxf_write_primer_pack(AVFormatContext *s)
         if (av_stream_get_side_data(s->streams[i], AV_PKT_DATA_MASTERING_DISPLAY_METADATA, NULL)) {
             will_have_mastering_tags = 1;
         }
+        if (s->streams[i]->codecpar->codec_id == AV_CODEC_ID_FFV1) {
+            will_have_ffv1_tags = 1;
+        }
     }
 
     if (!mxf->store_user_comments) {
@@ -541,8 +567,11 @@ static void mxf_write_primer_pack(AVFormatContext *s)
         mxf_mark_tag_unused(mxf, 0x5003);
     }
 
-    if (!will_have_avc_tags) {
+    if (!will_have_avc_tags && !will_have_ffv1_tags) {
         mxf_mark_tag_unused(mxf, 0x8100);
+    }
+
+    if (!will_have_avc_tags) {
         mxf_mark_tag_unused(mxf, 0x8200);
         mxf_mark_tag_unused(mxf, 0x8201);
         mxf_mark_tag_unused(mxf, 0x8202);
@@ -555,6 +584,12 @@ static void mxf_write_primer_pack(AVFormatContext *s)
         mxf_mark_tag_unused(mxf, 0x8304);
     }
 
+    if (!will_have_ffv1_tags) {
+        mxf_mark_tag_unused(mxf, 0xDFD9);
+        mxf_mark_tag_unused(mxf, 0xDFDA);
+        mxf_mark_tag_unused(mxf, 0xDFDB);
+    }
+
     for (i = 0; i < MXF_NUM_TAGS; i++) {
         if (mxf->unused_tags[i]) {
             local_tag_number--;
@@ -1091,9 +1126,11 @@ static const UID mxf_mpegvideo_descriptor_key = { 0x06,0x0E,0x2B,0x34,0x02,0x53,
 static const UID mxf_wav_descriptor_key       = { 0x06,0x0E,0x2B,0x34,0x02,0x53,0x01,0x01,0x0d,0x01,0x01,0x01,0x01,0x01,0x48,0x00 };
 static const UID mxf_aes3_descriptor_key      = { 0x06,0x0E,0x2B,0x34,0x02,0x53,0x01,0x01,0x0d,0x01,0x01,0x01,0x01,0x01,0x47,0x00 };
 static const UID mxf_cdci_descriptor_key      = { 0x06,0x0E,0x2B,0x34,0x02,0x53,0x01,0x01,0x0D,0x01,0x01,0x01,0x01,0x01,0x28,0x00 };
+static const UID mxf_rgba_descriptor_key      = { 0x06,0x0E,0x2B,0x34,0x02,0x53,0x01,0x01,0x0D,0x01,0x01,0x01,0x01,0x01,0x29,0x00 };
 static const UID mxf_generic_sound_descriptor_key = { 0x06,0x0E,0x2B,0x34,0x02,0x53,0x01,0x01,0x0D,0x01,0x01,0x01,0x01,0x01,0x42,0x00 };
 
 static const UID mxf_avc_subdescriptor_key = { 0x06,0x0E,0x2B,0x34,0x02,0x53,0x01,0x01,0x0d,0x01,0x01,0x01,0x01,0x01,0x6E,0x00 };
+static const UID mxf_ffv1_subdescriptor_key = { 0x06,0x0E,0x2B,0x34,0x02,0x53,0x01,0x01,0x0d,0x01,0x01,0x01,0x01,0x01,0x81,0x03 };
 
 static inline uint16_t rescale_mastering_chroma(AVRational q)
 {
@@ -1195,6 +1232,7 @@ static int64_t mxf_write_cdci_common(AVFormatContext *s, AVStream *st, const UID
         avio_wb32(pb, -((st->codecpar->height - display_height)&1));
     }
 
+    if (key != mxf_rgba_descriptor_key) {
     // component depth
     mxf_write_local_tag(s, 4, 0x3301);
     avio_wb32(pb, sc->component_depth);
@@ -1231,6 +1269,7 @@ static int64_t mxf_write_cdci_common(AVFormatContext *s, AVStream *st, const UID
         mxf_write_local_tag(s, 4, 0x3306);
         avio_wb32(pb, color);
     }
+    } // if (key != mxf_rgba_descriptor_key)
 
     if (sc->signal_standard) {
         mxf_write_local_tag(s, 1, 0x3215);
@@ -1326,6 +1365,13 @@ static int64_t mxf_write_cdci_common(AVFormatContext *s, AVStream *st, const UID
         mxf_write_uuid(pb, AVCSubDescriptor, 0);
     }
 
+    if (st->codecpar->codec_id == AV_CODEC_ID_FFV1) {
+        // write ffv1 sub descriptor ref
+        mxf_write_local_tag(s, 8 + 16, 0x8100);
+        mxf_write_refs_count(pb, 1);
+        mxf_write_uuid(pb, FFV1SubDescriptor, 0);
+    }
+
     return pos;
 }
 
@@ -1384,6 +1430,47 @@ static void mxf_write_h264_desc(AVFormatContext *s, AVStream *st)
     }
 }
 
+static void mxf_write_ffv1_subdesc(AVFormatContext *s, AVStream *st)
+{
+    AVIOContext *pb = s->pb;
+    MXFStreamContext *sc = st->priv_data;
+    int64_t pos;
+
+    avio_write(pb, mxf_ffv1_subdescriptor_key, 16);
+    klv_encode_ber4_length(pb, 0);
+    pos = avio_tell(pb);
+    
+    mxf_write_local_tag(s, 16, 0x3C0A);
+    mxf_write_uuid(pb, FFV1SubDescriptor, 0);
+
+    if (st->codecpar->extradata_size) {
+        mxf_write_local_tag(s, st->codecpar->extradata_size, 0xDFDB);
+        avio_write(pb, st->codecpar->extradata, st->codecpar->extradata_size); // FFV1InitializationMetadata
+    }
+
+    mxf_write_local_tag(s, 2, 0xDFDA);
+    avio_wb16(pb, (*sc->codec_ul)[14]); // FFV1Version
+
+    if (st->codecpar->extradata_size) {
+        mxf_write_local_tag(s, 2, 0xDFD9);
+        avio_wb16(pb, sc->micro_version); // FFV1MicroVersion
+    }
+
+    mxf_update_klv_size(s->pb, pos);
+}
+
+static void mxf_write_ffv1_desc(AVFormatContext *s, AVStream *st)
+{
+    int is_rgb, pos;
+    const AVPixFmtDescriptor *desc = av_pix_fmt_desc_get(st->codecpar->format);
+    av_assert0(desc);
+    is_rgb = desc->flags & AV_PIX_FMT_FLAG_RGB;
+
+    pos = mxf_write_cdci_common(s, st, is_rgb ? mxf_rgba_descriptor_key : mxf_cdci_descriptor_key);
+    mxf_update_klv_size(s->pb, pos);
+    mxf_write_ffv1_subdesc(s, st);
+}
+
 static void mxf_write_s436m_anc_desc(AVFormatContext *s, AVStream *st)
 {
     int64_t pos = mxf_write_generic_desc(s, st, mxf_s436m_anc_descriptor_key);
@@ -2344,6 +2431,73 @@ static int mxf_parse_h264_frame(AVFormatContext *s, AVStream *st,
     return 1;
 }
 
+static inline int get_ffv1_unsigned_symbol(RangeCoder *c, uint8_t *state) {
+    if(get_rac(c, state+0))
+        return 0;
+    else{
+        int i, e;
+        unsigned a;
+        e= 0;
+        while(get_rac(c, state+1 + FFMIN(e,9))){ //1..10
+            e++;
+            if (e > 31)
+                return AVERROR_INVALIDDATA;
+        }
+
+        a= 1;
+        for(i=e-1; i>=0; i--){
+            a += a + get_rac(c, state+22 + FFMIN(i,9)); //22..31
+        }
+
+        return a;
+    }
+}
+#define FFV1_CONTEXT_SIZE 32
+static int mxf_parse_ffv1_frame(AVFormatContext *s, AVStream *st, AVPacket *pkt)
+{
+    MXFContext *mxf = s->priv_data;
+    MXFStreamContext *sc = st->priv_data;
+    uint8_t state[FFV1_CONTEXT_SIZE];
+    RangeCoder c;
+    unsigned v;
+
+    sc->frame_size = pkt->size;
+
+    if (mxf->header_written)
+        return 1;
+
+    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);
+    }
+    sc->codec_ul = &mxf_ffv1_codec_uls[v];
+    
+    if (st->codecpar->field_order > AV_FIELD_PROGRESSIVE) {
+        sc->interlaced = 1;
+        sc->field_dominance = st->codecpar->field_order == (st->codecpar->field_order == AV_FIELD_TT || st->codecpar->field_order == AV_FIELD_TB) ? 1 : 2;
+    }
+    sc->aspect_ratio.num = st->codecpar->width * st->codecpar->sample_aspect_ratio.num;
+    sc->aspect_ratio.den = st->codecpar->height * st->codecpar->sample_aspect_ratio.den;
+    av_reduce(&sc->aspect_ratio.num, &sc->aspect_ratio.den,
+              sc->aspect_ratio.num, sc->aspect_ratio.den, INT_MAX);
+
+    return 1;
+}
+
 static const UID mxf_mpeg2_codec_uls[] = {
     { 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x03,0x04,0x01,0x02,0x02,0x01,0x01,0x10,0x00 }, // MP-ML I-Frame
     { 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x03,0x04,0x01,0x02,0x02,0x01,0x01,0x11,0x00 }, // MP-ML Long GOP
@@ -2955,6 +3109,11 @@ static int mxf_write_packet(AVFormatContext *s, AVPacket *pkt)
             av_log(s, AV_LOG_ERROR, "could not get h264 profile\n");
             return -1;
         }
+    } 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;
+        }
     }
 
     if (mxf->cbr_index) {
diff --git a/libavformat/rangecoder_dec.c b/libavformat/rangecoder_dec.c
new file mode 100644
index 0000000000..7b727f656e
--- /dev/null
+++ b/libavformat/rangecoder_dec.c
@@ -0,0 +1 @@
+#include "libavcodec/rangecoder.c"
-- 
2.13.3.windows.1


[-- Attachment #3: Type: text/plain, Size: 251 bytes --]

_______________________________________________
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] avformat/mxfenc: SMPTE RDD 48:2018 Amd 1:2022 (FFV1 in MXF) support

[Tomas Härdin]

lör 2023-01-14 klockan 16:45 +0100 skrev Jerome Martinez:
> The arbitrary short element codes are the ones used by another muxer
> ( 
> files available at 
> https://www.digitizationguidelines.gov/guidelines/MXF_app_sampleFiles.html#2022
>  
> )
> 
> The support of RGBA descriptor is added, mainly by disabling in the
> CDCI 
> descriptor related code the elements not in the Generic picture 
> descriptor, and could be in a separated dedicated patch (move of
> Generic 
> picture descriptor code in a dedicated function?).
> 
> Tested with:
> ffmpeg -f lavfi -i testsrc=duration=10:size=ntsc:rate=ntsc -
> field_order 
> bb -c:v ffv1 -level 0 test_ffv1_ntsc.mxf
> ffmpeg -f lavfi -i testsrc=duration=10:size=pal:rate=pal -field_order
> tt 
> -c:v ffv1 -level 3 test_ffv1_pal.mxf
> ffmpeg -f lavfi -i testsrc=duration=10:size=1920x1080 -pix_fmt
> yuv422p10 
> -c:v ffv1 -level 3 test_ffv1_hd.mxf

JPEG2000 will also need an RGBA descriptor filled out, might be good to
prepare for that.

The ffv1 parsing code in this patch makes me nervous. Isn't the version
available in metadata?

> +        ff_build_rac_states(&c, 0.05 * (1LL << 32), 256 - 8);

(1LL << 32) / 20 ?

/Tomas

_______________________________________________
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] avformat/mxfenc: SMPTE RDD 48:2018 Amd 1:2022 (FFV1 in MXF) support

[Jerome Martinez]

On 16/01/2023 15:00, Tomas Härdin wrote:
> JPEG2000 will also need an RGBA descriptor filled out, might be good to
> prepare for that.

this was the idea behind the way it is coded, so there is only a new 
mxf_write_jpeg2000_desc function to write, like the one for FFV1 i.e.
static void mxf_write_jpeg2000_desc(AVFormatContext *s, AVStream *st)
     is_rgb = desc->flags & AV_PIX_FMT_FLAG_RGB;
     pos = mxf_write_cdci_common(s, st, is_rgb ? mxf_rgba_descriptor_key 
: mxf_cdci_descriptor_key);
}

to add.


>
> The ffv1 parsing code in this patch makes me nervous. Isn't the version
> available in metadata?

I implemented a way similar to e.g. mxf_parse_mpeg2_frame by parsing a 
bit the frame.
version and micro_version are available in FFV1Context so could be used 
but I don't know how to get FFV1Context from AVStream or other, need 
help there.


>
>> +        ff_build_rac_states(&c, 0.05 * (1LL << 32), 256 - 8);
> (1LL << 32) / 20 ?

Could be, I don't really care, but this line is copied from ffv1dec.c, I 
think it may be relevant to keep the exact same code for the exact same 
purpose.
Would be no more relevant if version and micro_version can be taken from 
FFV1Context.

Jérôme
_______________________________________________
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] avformat/mxfenc: SMPTE RDD 48:2018 Amd 1:2022 (FFV1 in MXF) support

[Tomas Härdin]

mån 2023-01-16 klockan 15:17 +0100 skrev Jerome Martinez:
> On 16/01/2023 15:00, Tomas Härdin wrote:
> > 
> > > +        ff_build_rac_states(&c, 0.05 * (1LL << 32), 256 - 8);
> > (1LL << 32) / 20 ?
> 
> Could be, I don't really care, but this line is copied from ffv1dec.c

Yeah I figured as much after doing some more grepping

> I 
> think it may be relevant to keep the exact same code for the exact
> same 
> purpose.
> Would be no more relevant if version and micro_version can be taken
> from 
> FFV1Context.

Perhaps we can have the ffv1 code in lavc expose a function for this? I
don't really like that we have this kind of parsing inside the muxers.
This goes for MPEG-2 and H.264 as well.

/Tomas

_______________________________________________
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] avformat/mxfenc: SMPTE RDD 48:2018 Amd 1:2022 (FFV1 in MXF) support

[Jerome Martinez]

On 18/01/2023 11:12, Tomas Härdin wrote:
> mån 2023-01-16 klockan 15:17 +0100 skrev Jerome Martinez:
> [...]
>> I
>> think it may be relevant to keep the exact same code for the exact
>> same
>> purpose.
>> Would be no more relevant if version and micro_version can be taken
>> from
>> FFV1Context.
> Perhaps we can have the ffv1 code in lavc expose a function for this? I
> don't really like that we have this kind of parsing inside the muxers.
> This goes for MPEG-2 and H.264 as well.

I don't like that too but it is beyond my skills, and as this patch does 
not do worse that what is already implemented, with a very small 
overhead (less that the MPEG-2 and H.264 parts which were accepted to be 
merged as is), I suggest that this is not a blocker here and this part 
of the code could be trashed when lavc is able to expose the codec 
context (if it is not yet able to do so).
_______________________________________________
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] avformat/mxfenc: SMPTE RDD 48:2018 Amd 1:2022 (FFV1 in MXF) support

[Tomas Härdin]

Creating a new subthread because I just noticed something

> +    //Stored height
>      mxf_write_local_tag(s, 4, 0x3202);
>      avio_wb32(pb, stored_height>>sc->interlaced);
> 

Won't this be incorrect for files whose dimensions are multiples of 16
but not multiples of 32? Isn't each field stored separately with
dimensions a multiple of 16? So while for 1080p we'll have

  StoredHeight = 1088
  SampledHeight = 1080

and 1080i:

  StoredHeight = 544
  SampledHeight = 540

Where 544 is a multiple of 16, for say 720p we have

  StoredHeight = 720
  SampledHeight = 720

but for a hypothetical 720i we'd get

  StoredHeight = 360
  SampledHeight = 360

whereas the correct values should be

  StoredHeight = 368
  SampledHeight = 360

?

/Tomas

_______________________________________________
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] avformat/mxfenc: SMPTE RDD 48:2018 Amd 1:2022 (FFV1 in MXF) support

[Jerome Martinez]

On 18/01/2023 14:40, Tomas Härdin wrote:
> Creating a new subthread because I just noticed something

I am a bit lost there because the line of code below is not part of this 
FFV1 patch.
Additionally, none on my patches (FFV1 of MXF stored/sampled/displayed 
fix) modifies the discussed behavior (FFmpeg behavior would be same 
before and after this patch for MPEG-2 and AVC), so should not block any 
of them, and a potential fix for that should have its own patch as it 
would be a separate issue.

Anyway:


>
>> +    //Stored height
>>       mxf_write_local_tag(s, 4, 0x3202);
>>       avio_wb32(pb, stored_height>>sc->interlaced);
>>
> Won't this be incorrect for files whose dimensions are multiples of 16
> but not multiples of 32? Isn't each field stored separately with
> dimensions a multiple of 16? So while for 1080p we'll have
>
>    StoredHeight = 1088
>    SampledHeight = 1080
>
> and 1080i:
>
>    StoredHeight = 544
>    SampledHeight = 540
>
> Where 544 is a multiple of 16, for say 720p we have
>
>    StoredHeight = 720
>    SampledHeight = 720
>
> but for a hypothetical 720i we'd get
>
>    StoredHeight = 360
>    SampledHeight = 360
>
> whereas the correct values should be
>
>    StoredHeight = 368
>    SampledHeight = 360

AFAIK, it would depend about if the stream has a picture_structure frame 
(16x16 applies to the frame?) of field (16x16 applies to the field?), 
but I really don't know enough there for having a relevant opinion.

I can just say that I don't change the behavior of FFmpeg in your use 
case, I found the issues when I tried a random width and height of FFV1 
stream then checked with MPEG-2 Video and the sampled width was wrong 
for sure e.g. sampled width of 1920 for a stream having a width of 1912, 
with current FFmpeg code, and for your use case I am sure about nothing 
so I don't change the behavior with my patch, IMO if there is an issue 
with 720i MPEG-2 Video it should be in a separate topic and patch as it 
would modify the "stored_height = (st->codecpar->height+15)/16*16" 
current code (in my patch I just move this code), unless we are sure of 
what should be changed on this side and apply a fix on the way. Better 
to fix 1 issue and let 1 open with no change than fixing no issue 
because we wouldn't be sure for 1 of the 2.

Jérôme

_______________________________________________
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] avformat/mxfenc: SMPTE RDD 48:2018 Amd 1:2022 (FFV1 in MXF) support

[Tomas Härdin]

ons 2023-01-18 klockan 15:15 +0100 skrev Jerome Martinez:
> On 18/01/2023 14:40, Tomas Härdin wrote:
> > Creating a new subthread because I just noticed something
> 
> I am a bit lost there because the line of code below is not part of
> this 
> FFV1 patch.
> Additionally, none on my patches (FFV1 of MXF
> stored/sampled/displayed 
> fix) modifies the discussed behavior (FFmpeg behavior would be same 
> before and after this patch for MPEG-2 and AVC), so should not block
> any 
> of them, and a potential fix for that should have its own patch as it
> would be a separate issue.

True, it doesn't need to hold up this patch. But some discussion is
warranted I think. I might create a separate patchset for this.

> 
> Anyway:
> 
> 
> > 
> > > +    //Stored height
> > >       mxf_write_local_tag(s, 4, 0x3202);
> > >       avio_wb32(pb, stored_height>>sc->interlaced);
> > > 
> > Won't this be incorrect for files whose dimensions are multiples of
> > 16
> > but not multiples of 32? Isn't each field stored separately with
> > dimensions a multiple of 16? So while for 1080p we'll have
> > 
> >    StoredHeight = 1088
> >    SampledHeight = 1080
> > 
> > and 1080i:
> > 
> >    StoredHeight = 544
> >    SampledHeight = 540
> > 
> > Where 544 is a multiple of 16, for say 720p we have
> > 
> >    StoredHeight = 720
> >    SampledHeight = 720
> > 
> > but for a hypothetical 720i we'd get
> > 
> >    StoredHeight = 360
> >    SampledHeight = 360
> > 
> > whereas the correct values should be
> > 
> >    StoredHeight = 368
> >    SampledHeight = 360
> 
> AFAIK, it would depend about if the stream has a picture_structure
> frame 
> (16x16 applies to the frame?) of field (16x16 applies to the field?),
> but I really don't know enough there for having a relevant opinion.
> 
> I can just say that I don't change the behavior of FFmpeg in your use
> case, I found the issues when I tried a random width and height of
> FFV1 
> stream then checked with MPEG-2 Video and the sampled width was wrong
> for sure e.g. sampled width of 1920 for a stream having a width of
> 1912, 
> with current FFmpeg code, and for your use case I am sure about
> nothing 
> so I don't change the behavior with my patch, IMO if there is an
> issue 
> with 720i MPEG-2 Video it should be in a separate topic and patch as
> it 
> would modify the "stored_height = (st->codecpar->height+15)/16*16" 
> current code (in my patch I just move this code), unless we are sure
> of 
> what should be changed on this side and apply a fix on the way.
> Better 
> to fix 1 issue and let 1 open with no change than fixing no issue 
> because we wouldn't be sure for 1 of the 2.

I suspect we are lucky because 720i doesn't really exist in the real
world, and 576i and 480i are both multiples of 32.

IMO mxfenc shouldn't lie, but looking at S377m StoredWidth/Height are
"best effort" and thus shall be encoded. Their values will depend on
FrameLayout which in turn depends on what you say - how exactly the
interlacing is done.

TL;DR: this patchset doesn't need to be held up by this.

/Tomas

_______________________________________________
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] avformat/mxfenc: SMPTE RDD 48:2018 Amd 1:2022 (FFV1 in MXF) support

[Dave Rice]

> On Jan 20, 2023, at 10:17 AM, Tomas Härdin <git@haerdin.se> wrote:
> 
> ons 2023-01-18 klockan 15:15 +0100 skrev Jerome Martinez:
>> On 18/01/2023 14:40, Tomas Härdin wrote:
>>> Creating a new subthread because I just noticed something
>> 
>> I am a bit lost there because the line of code below is not part of
>> this 
>> FFV1 patch.
>> Additionally, none on my patches (FFV1 of MXF
>> stored/sampled/displayed 
>> fix) modifies the discussed behavior (FFmpeg behavior would be same 
>> before and after this patch for MPEG-2 and AVC), so should not block
>> any 
>> of them, and a potential fix for that should have its own patch as it
>> would be a separate issue.
> 
> True, it doesn't need to hold up this patch. But some discussion is
> warranted I think. I might create a separate patchset for this.
> 
>> 
>> Anyway:
>> 
>> 
>>> 
>>>> +    //Stored height
>>>>       mxf_write_local_tag(s, 4, 0x3202);
>>>>       avio_wb32(pb, stored_height>>sc->interlaced);
>>>> 
>>> Won't this be incorrect for files whose dimensions are multiples of
>>> 16
>>> but not multiples of 32? Isn't each field stored separately with
>>> dimensions a multiple of 16? So while for 1080p we'll have
>>> 
>>>    StoredHeight = 1088
>>>    SampledHeight = 1080
>>> 
>>> and 1080i:
>>> 
>>>    StoredHeight = 544
>>>    SampledHeight = 540
>>> 
>>> Where 544 is a multiple of 16, for say 720p we have
>>> 
>>>    StoredHeight = 720
>>>    SampledHeight = 720
>>> 
>>> but for a hypothetical 720i we'd get
>>> 
>>>    StoredHeight = 360
>>>    SampledHeight = 360
>>> 
>>> whereas the correct values should be
>>> 
>>>    StoredHeight = 368
>>>    SampledHeight = 360
>> 
>> AFAIK, it would depend about if the stream has a picture_structure
>> frame 
>> (16x16 applies to the frame?) of field (16x16 applies to the field?),
>> but I really don't know enough there for having a relevant opinion.
>> 
>> I can just say that I don't change the behavior of FFmpeg in your use
>> case, I found the issues when I tried a random width and height of
>> FFV1 
>> stream then checked with MPEG-2 Video and the sampled width was wrong
>> for sure e.g. sampled width of 1920 for a stream having a width of
>> 1912, 
>> with current FFmpeg code, and for your use case I am sure about
>> nothing 
>> so I don't change the behavior with my patch, IMO if there is an
>> issue 
>> with 720i MPEG-2 Video it should be in a separate topic and patch as
>> it 
>> would modify the "stored_height = (st->codecpar->height+15)/16*16" 
>> current code (in my patch I just move this code), unless we are sure
>> of 
>> what should be changed on this side and apply a fix on the way.
>> Better 
>> to fix 1 issue and let 1 open with no change than fixing no issue 
>> because we wouldn't be sure for 1 of the 2.
> 
> I suspect we are lucky because 720i doesn't really exist in the real
> world, and 576i and 480i are both multiples of 32.
> 
> IMO mxfenc shouldn't lie, but looking at S377m StoredWidth/Height are
> "best effort" and thus shall be encoded. Their values will depend on
> FrameLayout which in turn depends on what you say - how exactly the
> interlacing is done.
> 
> TL;DR: this patchset doesn't need to be held up by this.

I'm just nudging on the consideration of merging this patch. I've been testing it over the last week with ffv1/mxf content and have found this demuxing support very helpful.
Dave Rice
_______________________________________________
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] avformat/mxfenc: SMPTE RDD 48:2018 Amd 1:2022 (FFV1 in MXF) support

[Tomas Härdin]

sön 2023-01-29 klockan 11:36 -0500 skrev Dave Rice:
> 
> 
> > On Jan 20, 2023, at 10:17 AM, Tomas Härdin <git@haerdin.se> wrote:
> > 
> > ons 2023-01-18 klockan 15:15 +0100 skrev Jerome Martinez:
> > > On 18/01/2023 14:40, Tomas Härdin wrote:
> > > > Creating a new subthread because I just noticed something
> > > 
> > > I am a bit lost there because the line of code below is not part
> > > of
> > > this 
> > > FFV1 patch.
> > > Additionally, none on my patches (FFV1 of MXF
> > > stored/sampled/displayed 
> > > fix) modifies the discussed behavior (FFmpeg behavior would be
> > > same 
> > > before and after this patch for MPEG-2 and AVC), so should not
> > > block
> > > any 
> > > of them, and a potential fix for that should have its own patch
> > > as it
> > > would be a separate issue.
> > 
> > True, it doesn't need to hold up this patch. But some discussion is
> > warranted I think. I might create a separate patchset for this.
> > 
> > > 
> > > Anyway:
> > > 
> > > 
> > > > 
> > > > > +    //Stored height
> > > > >       mxf_write_local_tag(s, 4, 0x3202);
> > > > >       avio_wb32(pb, stored_height>>sc->interlaced);
> > > > > 
> > > > Won't this be incorrect for files whose dimensions are
> > > > multiples of
> > > > 16
> > > > but not multiples of 32? Isn't each field stored separately
> > > > with
> > > > dimensions a multiple of 16? So while for 1080p we'll have
> > > > 
> > > >    StoredHeight = 1088
> > > >    SampledHeight = 1080
> > > > 
> > > > and 1080i:
> > > > 
> > > >    StoredHeight = 544
> > > >    SampledHeight = 540
> > > > 
> > > > Where 544 is a multiple of 16, for say 720p we have
> > > > 
> > > >    StoredHeight = 720
> > > >    SampledHeight = 720
> > > > 
> > > > but for a hypothetical 720i we'd get
> > > > 
> > > >    StoredHeight = 360
> > > >    SampledHeight = 360
> > > > 
> > > > whereas the correct values should be
> > > > 
> > > >    StoredHeight = 368
> > > >    SampledHeight = 360
> > > 
> > > AFAIK, it would depend about if the stream has a
> > > picture_structure
> > > frame 
> > > (16x16 applies to the frame?) of field (16x16 applies to the
> > > field?),
> > > but I really don't know enough there for having a relevant
> > > opinion.
> > > 
> > > I can just say that I don't change the behavior of FFmpeg in your
> > > use
> > > case, I found the issues when I tried a random width and height
> > > of
> > > FFV1 
> > > stream then checked with MPEG-2 Video and the sampled width was
> > > wrong
> > > for sure e.g. sampled width of 1920 for a stream having a width
> > > of
> > > 1912, 
> > > with current FFmpeg code, and for your use case I am sure about
> > > nothing 
> > > so I don't change the behavior with my patch, IMO if there is an
> > > issue 
> > > with 720i MPEG-2 Video it should be in a separate topic and patch
> > > as
> > > it 
> > > would modify the "stored_height = (st->codecpar-
> > > >height+15)/16*16" 
> > > current code (in my patch I just move this code), unless we are
> > > sure
> > > of 
> > > what should be changed on this side and apply a fix on the way.
> > > Better 
> > > to fix 1 issue and let 1 open with no change than fixing no issue
> > > because we wouldn't be sure for 1 of the 2.
> > 
> > I suspect we are lucky because 720i doesn't really exist in the
> > real
> > world, and 576i and 480i are both multiples of 32.
> > 
> > IMO mxfenc shouldn't lie, but looking at S377m StoredWidth/Height
> > are
> > "best effort" and thus shall be encoded. Their values will depend
> > on
> > FrameLayout which in turn depends on what you say - how exactly the
> > interlacing is done.
> > 
> > TL;DR: this patchset doesn't need to be held up by this.
> 
> I'm just nudging on the consideration of merging this patch. I've
> been testing it over the last week with ffv1/mxf content and have
> found this demuxing support very helpful.

Surely you mean muxing?

Some FATE tests would be nice.

/Tomas

_______________________________________________
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] avformat/mxfenc: SMPTE RDD 48:2018 Amd 1:2022 (FFV1 in MXF) support

[Jerome Martinez]

[-- Attachment #1: Type: text/plain, Size: 3889 bytes --]

On 31/01/2023 15:53, Tomas Härdin wrote:
> sön 2023-01-29 klockan 11:36 -0500 skrev Dave Rice:
>>
>>> On Jan 20, 2023, at 10:17 AM, Tomas Härdin <git@haerdin.se> wrote:
>>>
>>> ons 2023-01-18 klockan 15:15 +0100 skrev Jerome Martinez:
>>>> On 18/01/2023 14:40, Tomas Härdin wrote:
>>>>> Creating a new subthread because I just noticed something
>>>> I am a bit lost there because the line of code below is not part
>>>> of
>>>> this
>>>> FFV1 patch.
>>>> Additionally, none on my patches (FFV1 of MXF
>>>> stored/sampled/displayed
>>>> fix) modifies the discussed behavior (FFmpeg behavior would be
>>>> same
>>>> before and after this patch for MPEG-2 and AVC), so should not
>>>> block
>>>> any
>>>> of them, and a potential fix for that should have its own patch
>>>> as it
>>>> would be a separate issue.
>>> True, it doesn't need to hold up this patch. But some discussion is
>>> warranted I think. I might create a separate patchset for this.
>>>
>>>> Anyway:
>>>>
>>>>
>>>>>> +    //Stored height
>>>>>>        mxf_write_local_tag(s, 4, 0x3202);
>>>>>>        avio_wb32(pb, stored_height>>sc->interlaced);
>>>>>>
>>>>> Won't this be incorrect for files whose dimensions are
>>>>> multiples of
>>>>> 16
>>>>> but not multiples of 32? Isn't each field stored separately
>>>>> with
>>>>> dimensions a multiple of 16? So while for 1080p we'll have
>>>>>
>>>>>     StoredHeight = 1088
>>>>>     SampledHeight = 1080
>>>>>
>>>>> and 1080i:
>>>>>
>>>>>     StoredHeight = 544
>>>>>     SampledHeight = 540
>>>>>
>>>>> Where 544 is a multiple of 16, for say 720p we have
>>>>>
>>>>>     StoredHeight = 720
>>>>>     SampledHeight = 720
>>>>>
>>>>> but for a hypothetical 720i we'd get
>>>>>
>>>>>     StoredHeight = 360
>>>>>     SampledHeight = 360
>>>>>
>>>>> whereas the correct values should be
>>>>>
>>>>>     StoredHeight = 368
>>>>>     SampledHeight = 360
>>>> AFAIK, it would depend about if the stream has a
>>>> picture_structure
>>>> frame
>>>> (16x16 applies to the frame?) of field (16x16 applies to the
>>>> field?),
>>>> but I really don't know enough there for having a relevant
>>>> opinion.
>>>>
>>>> I can just say that I don't change the behavior of FFmpeg in your
>>>> use
>>>> case, I found the issues when I tried a random width and height
>>>> of
>>>> FFV1
>>>> stream then checked with MPEG-2 Video and the sampled width was
>>>> wrong
>>>> for sure e.g. sampled width of 1920 for a stream having a width
>>>> of
>>>> 1912,
>>>> with current FFmpeg code, and for your use case I am sure about
>>>> nothing
>>>> so I don't change the behavior with my patch, IMO if there is an
>>>> issue
>>>> with 720i MPEG-2 Video it should be in a separate topic and patch
>>>> as
>>>> it
>>>> would modify the "stored_height = (st->codecpar-
>>>>> height+15)/16*16"
>>>> current code (in my patch I just move this code), unless we are
>>>> sure
>>>> of
>>>> what should be changed on this side and apply a fix on the way.
>>>> Better
>>>> to fix 1 issue and let 1 open with no change than fixing no issue
>>>> because we wouldn't be sure for 1 of the 2.
>>> I suspect we are lucky because 720i doesn't really exist in the
>>> real
>>> world, and 576i and 480i are both multiples of 32.
>>>
>>> IMO mxfenc shouldn't lie, but looking at S377m StoredWidth/Height
>>> are
>>> "best effort" and thus shall be encoded. Their values will depend
>>> on
>>> FrameLayout which in turn depends on what you say - how exactly the
>>> interlacing is done.
>>>
>>> TL;DR: this patchset doesn't need to be held up by this.
>> I'm just nudging on the consideration of merging this patch. I've
>> been testing it over the last week with ffv1/mxf content and have
>> found this demuxing support very helpful.
> Surely you mean muxing?
>
> Some FATE tests would be nice.


Apologizes for the huge delay.
Attached is an updated patch with a FATE test.

Jérôme

[-- Attachment #2: 0001-avformat-mxfenc-SMPTE-RDD-48-2018-Amd-1-2022-support.patch --]
[-- Type: text/plain, Size: 17148 bytes --]

From fec4b918dd6e6a067eaeb2cd27f5e42c08dcca2c Mon Sep 17 00:00:00 2001
From: Jerome Martinez <jerome@mediaarea.net>
Date: Tue, 14 Mar 2023 09:49:16 +0100
Subject: [PATCH] avformat/mxfenc: SMPTE RDD 48:2018 Amd 1:2022 support

---
 libavformat/Makefile          |   3 +-
 libavformat/mxfenc.c          | 163 +++++++++++++++++++++++++++++++++++++++++-
 libavformat/rangecoder_dec.c  |   1 +
 tests/fate/lavf-container.mak |   2 +
 tests/ref/lavf/mxf_ffv1       |   3 +
 5 files changed, 169 insertions(+), 3 deletions(-)
 create mode 100644 libavformat/rangecoder_dec.c
 create mode 100644 tests/ref/lavf/mxf_ffv1

diff --git a/libavformat/Makefile b/libavformat/Makefile
index 47bbbbfb2a..048649689b 100644
--- a/libavformat/Makefile
+++ b/libavformat/Makefile
@@ -712,7 +712,8 @@ SHLIBOBJS-$(CONFIG_HLS_DEMUXER)          += ac3_channel_layout_tab.o
 SHLIBOBJS-$(CONFIG_MATROSKA_DEMUXER)     += mpeg4audio_sample_rates.o
 SHLIBOBJS-$(CONFIG_MOV_DEMUXER)          += ac3_channel_layout_tab.o
 SHLIBOBJS-$(CONFIG_MP3_MUXER)            += mpegaudiotabs.o
-SHLIBOBJS-$(CONFIG_MXF_MUXER)            += golomb_tab.o
+SHLIBOBJS-$(CONFIG_MXF_MUXER)            += golomb_tab.o \
+                                            rangecoder_dec.o
 SHLIBOBJS-$(CONFIG_NUT_MUXER)            += mpegaudiotabs.o
 SHLIBOBJS-$(CONFIG_RTPDEC)               += jpegtables.o
 SHLIBOBJS-$(CONFIG_RTP_MUXER)            += golomb_tab.o jpegtables.o \
diff --git a/libavformat/mxfenc.c b/libavformat/mxfenc.c
index a29d678098..0ff566fbb4 100644
--- a/libavformat/mxfenc.c
+++ b/libavformat/mxfenc.c
@@ -51,6 +51,7 @@
 #include "libavcodec/golomb.h"
 #include "libavcodec/h264.h"
 #include "libavcodec/packet_internal.h"
+#include "libavcodec/rangecoder.h"
 #include "libavcodec/startcode.h"
 #include "avformat.h"
 #include "avio_internal.h"
@@ -102,6 +103,7 @@ typedef struct MXFStreamContext {
     int b_picture_count;     ///< maximum number of consecutive b pictures, used in mpeg-2 descriptor
     int low_delay;           ///< low delay, used in mpeg-2 descriptor
     int avc_intra;
+    int micro_version;       ///< format micro_version, used in ffv1 descriptor
 } MXFStreamContext;
 
 typedef struct MXFContainerEssenceEntry {
@@ -130,6 +132,7 @@ enum ULIndex {
     INDEX_H264,
     INDEX_S436M,
     INDEX_PRORES,
+    INDEX_FFV1,
 };
 
 static const struct {
@@ -144,6 +147,7 @@ static const struct {
     { AV_CODEC_ID_JPEG2000,   INDEX_JPEG2000 },
     { AV_CODEC_ID_H264,       INDEX_H264 },
     { AV_CODEC_ID_PRORES,     INDEX_PRORES },
+    { AV_CODEC_ID_FFV1,       INDEX_FFV1 },
     { AV_CODEC_ID_NONE }
 };
 
@@ -151,6 +155,7 @@ static void mxf_write_wav_desc(AVFormatContext *s, AVStream *st);
 static void mxf_write_aes3_desc(AVFormatContext *s, AVStream *st);
 static void mxf_write_mpegvideo_desc(AVFormatContext *s, AVStream *st);
 static void mxf_write_h264_desc(AVFormatContext *s, AVStream *st);
+static void mxf_write_ffv1_desc(AVFormatContext *s, AVStream *st);
 static void mxf_write_cdci_desc(AVFormatContext *s, AVStream *st);
 static void mxf_write_generic_sound_desc(AVFormatContext *s, AVStream *st);
 static void mxf_write_s436m_anc_desc(AVFormatContext *s, AVStream *st);
@@ -208,6 +213,11 @@ static const MXFContainerEssenceEntry mxf_essence_container_uls[] = {
       { 0x06,0x0E,0x2B,0x34,0x01,0x02,0x01,0x01,0x0d,0x01,0x03,0x01,0x15,0x01,0x17,0x00 },
       { 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x0d,0x04,0x01,0x02,0x02,0x03,0x06,0x03,0x00 },
       mxf_write_cdci_desc },
+    // FFV1
+    { { 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x0d,0x0d,0x01,0x03,0x01,0x02,0x23,0x01,0x00 },
+      { 0x06,0x0E,0x2B,0x34,0x01,0x02,0x01,0x01,0x0d,0x01,0x03,0x01,0x15,0x01,0x1d,0x00 },
+      { 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x0d,0x04,0x01,0x02,0x02,0x03,0x09,0x00,0x00 },
+      mxf_write_ffv1_desc },
     { { 0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00 },
       { 0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00 },
       { 0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00 },
@@ -232,6 +242,15 @@ static const UID mxf_d10_container_uls[] = {
     { 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x01,0x0D,0x01,0x03,0x01,0x02,0x01,0x06,0x01 }, // D-10 525/50 NTSC 30mb/s
 };
 
+
+static const UID mxf_ffv1_codec_uls[] = {
+    { 0x06,0x0e,0x2b,0x34,0x04,0x01,0x01,0x0D,0x04,0x01,0x02,0x02,0x03,0x09,0x01,0x00 }, // FFV1 version 0
+    { 0x06,0x0e,0x2b,0x34,0x04,0x01,0x01,0x0D,0x04,0x01,0x02,0x02,0x03,0x09,0x02,0x00 }, // FFV1 version 1
+    { 0x06,0x0e,0x2b,0x34,0x04,0x01,0x01,0x0D,0x04,0x01,0x02,0x02,0x03,0x09,0x03,0x00 }, // FFV1 version 2 (was only experimental)
+    { 0x06,0x0e,0x2b,0x34,0x04,0x01,0x01,0x0D,0x04,0x01,0x02,0x02,0x03,0x09,0x04,0x00 }, // FFV1 version 3
+    { 0x06,0x0e,0x2b,0x34,0x04,0x01,0x01,0x0D,0x04,0x01,0x02,0x02,0x03,0x09,0x05,0x00 }, // FFV1 version 4
+};
+
 static const uint8_t product_uid[]          = { 0xAD,0xAB,0x44,0x24,0x2f,0x25,0x4d,0xc7,0x92,0xff,0x29,0xbd,0x00,0x0c,0x00,0x02};
 static const uint8_t uuid_base[]            = { 0xAD,0xAB,0x44,0x24,0x2f,0x25,0x4d,0xc7,0x92,0xff };
 static const uint8_t umid_ul[]              = { 0x06,0x0A,0x2B,0x34,0x01,0x01,0x01,0x05,0x01,0x01,0x0D,0x00,0x13 };
@@ -390,6 +409,10 @@ static const MXFLocalTagPair mxf_local_tag_batch[] = {
     { 0x8302, FF_MXF_MasteringDisplayWhitePointChromaticity },
     { 0x8303, FF_MXF_MasteringDisplayMaximumLuminance },
     { 0x8304, FF_MXF_MasteringDisplayMinimumLuminance },
+    // FFV1
+    { 0xDFD9, {0x06,0x0E,0x2B,0x34,0x01,0x01,0x01,0x0E,0x04,0x01,0x06,0x0C,0x06,0x00,0x00,0x00}}, /* FFV1 Micro-version */
+    { 0xDFDA, {0x06,0x0E,0x2B,0x34,0x01,0x01,0x01,0x0E,0x04,0x01,0x06,0x0C,0x05,0x00,0x00,0x00}}, /* FFV1 Version */
+    { 0xDFDB, {0x06,0x0E,0x2B,0x34,0x01,0x01,0x01,0x0E,0x04,0x01,0x06,0x0C,0x01,0x00,0x00,0x00}}, /* FFV1 Initialization Metadata */
 };
 
 #define MXF_NUM_TAGS FF_ARRAY_ELEMS(mxf_local_tag_batch)
@@ -526,7 +549,7 @@ static void mxf_write_primer_pack(AVFormatContext *s)
     MXFContext *mxf = s->priv_data;
     AVIOContext *pb = s->pb;
     int local_tag_number = MXF_NUM_TAGS, i;
-    int will_have_avc_tags = 0, will_have_mastering_tags = 0;
+    int will_have_avc_tags = 0, will_have_mastering_tags = 0, will_have_ffv1_tags = 0;
 
     for (i = 0; i < s->nb_streams; i++) {
         MXFStreamContext *sc = s->streams[i]->priv_data;
@@ -536,6 +559,9 @@ static void mxf_write_primer_pack(AVFormatContext *s)
         if (av_stream_get_side_data(s->streams[i], AV_PKT_DATA_MASTERING_DISPLAY_METADATA, NULL)) {
             will_have_mastering_tags = 1;
         }
+        if (s->streams[i]->codecpar->codec_id == AV_CODEC_ID_FFV1) {
+            will_have_ffv1_tags = 1;
+        }
     }
 
     if (!mxf->store_user_comments) {
@@ -544,8 +570,11 @@ static void mxf_write_primer_pack(AVFormatContext *s)
         mxf_mark_tag_unused(mxf, 0x5003);
     }
 
-    if (!will_have_avc_tags) {
+    if (!will_have_avc_tags && !will_have_ffv1_tags) {
         mxf_mark_tag_unused(mxf, 0x8100);
+    }
+
+    if (!will_have_avc_tags) {
         mxf_mark_tag_unused(mxf, 0x8200);
         mxf_mark_tag_unused(mxf, 0x8201);
         mxf_mark_tag_unused(mxf, 0x8202);
@@ -558,6 +587,12 @@ static void mxf_write_primer_pack(AVFormatContext *s)
         mxf_mark_tag_unused(mxf, 0x8304);
     }
 
+    if (!will_have_ffv1_tags) {
+        mxf_mark_tag_unused(mxf, 0xDFD9);
+        mxf_mark_tag_unused(mxf, 0xDFDA);
+        mxf_mark_tag_unused(mxf, 0xDFDB);
+    }
+
     for (i = 0; i < MXF_NUM_TAGS; i++) {
         if (mxf->unused_tags[i]) {
             local_tag_number--;
@@ -1094,9 +1129,11 @@ static const UID mxf_mpegvideo_descriptor_key = { 0x06,0x0E,0x2B,0x34,0x02,0x53,
 static const UID mxf_wav_descriptor_key       = { 0x06,0x0E,0x2B,0x34,0x02,0x53,0x01,0x01,0x0d,0x01,0x01,0x01,0x01,0x01,0x48,0x00 };
 static const UID mxf_aes3_descriptor_key      = { 0x06,0x0E,0x2B,0x34,0x02,0x53,0x01,0x01,0x0d,0x01,0x01,0x01,0x01,0x01,0x47,0x00 };
 static const UID mxf_cdci_descriptor_key      = { 0x06,0x0E,0x2B,0x34,0x02,0x53,0x01,0x01,0x0D,0x01,0x01,0x01,0x01,0x01,0x28,0x00 };
+static const UID mxf_rgba_descriptor_key      = { 0x06,0x0E,0x2B,0x34,0x02,0x53,0x01,0x01,0x0D,0x01,0x01,0x01,0x01,0x01,0x29,0x00 };
 static const UID mxf_generic_sound_descriptor_key = { 0x06,0x0E,0x2B,0x34,0x02,0x53,0x01,0x01,0x0D,0x01,0x01,0x01,0x01,0x01,0x42,0x00 };
 
 static const UID mxf_avc_subdescriptor_key = { 0x06,0x0E,0x2B,0x34,0x02,0x53,0x01,0x01,0x0d,0x01,0x01,0x01,0x01,0x01,0x6E,0x00 };
+static const UID mxf_ffv1_subdescriptor_key = { 0x06,0x0E,0x2B,0x34,0x02,0x53,0x01,0x01,0x0d,0x01,0x01,0x01,0x01,0x01,0x81,0x03 };
 
 static inline uint16_t rescale_mastering_chroma(AVRational q)
 {
@@ -1198,6 +1235,7 @@ static int64_t mxf_write_cdci_common(AVFormatContext *s, AVStream *st, const UID
         avio_wb32(pb, -((st->codecpar->height - display_height)&1));
     }
 
+    if (key != mxf_rgba_descriptor_key) {
     // component depth
     mxf_write_local_tag(s, 4, 0x3301);
     avio_wb32(pb, sc->component_depth);
@@ -1234,6 +1272,7 @@ static int64_t mxf_write_cdci_common(AVFormatContext *s, AVStream *st, const UID
         mxf_write_local_tag(s, 4, 0x3306);
         avio_wb32(pb, color);
     }
+    } // if (key != mxf_rgba_descriptor_key)
 
     if (sc->signal_standard) {
         mxf_write_local_tag(s, 1, 0x3215);
@@ -1329,6 +1368,13 @@ static int64_t mxf_write_cdci_common(AVFormatContext *s, AVStream *st, const UID
         mxf_write_uuid(pb, AVCSubDescriptor, 0);
     }
 
+    if (st->codecpar->codec_id == AV_CODEC_ID_FFV1) {
+        // write ffv1 sub descriptor ref
+        mxf_write_local_tag(s, 8 + 16, 0x8100);
+        mxf_write_refs_count(pb, 1);
+        mxf_write_uuid(pb, FFV1SubDescriptor, 0);
+    }
+
     return pos;
 }
 
@@ -1387,6 +1433,47 @@ static void mxf_write_h264_desc(AVFormatContext *s, AVStream *st)
     }
 }
 
+static void mxf_write_ffv1_subdesc(AVFormatContext *s, AVStream *st)
+{
+    AVIOContext *pb = s->pb;
+    MXFStreamContext *sc = st->priv_data;
+    int64_t pos;
+
+    avio_write(pb, mxf_ffv1_subdescriptor_key, 16);
+    klv_encode_ber4_length(pb, 0);
+    pos = avio_tell(pb);
+    
+    mxf_write_local_tag(s, 16, 0x3C0A);
+    mxf_write_uuid(pb, FFV1SubDescriptor, 0);
+
+    if (st->codecpar->extradata_size) {
+        mxf_write_local_tag(s, st->codecpar->extradata_size, 0xDFDB);
+        avio_write(pb, st->codecpar->extradata, st->codecpar->extradata_size); // FFV1InitializationMetadata
+    }
+
+    mxf_write_local_tag(s, 2, 0xDFDA);
+    avio_wb16(pb, (*sc->codec_ul)[14]); // FFV1Version
+
+    if (st->codecpar->extradata_size) {
+        mxf_write_local_tag(s, 2, 0xDFD9);
+        avio_wb16(pb, sc->micro_version); // FFV1MicroVersion
+    }
+
+    mxf_update_klv_size(s->pb, pos);
+}
+
+static void mxf_write_ffv1_desc(AVFormatContext *s, AVStream *st)
+{
+    int is_rgb, pos;
+    const AVPixFmtDescriptor *desc = av_pix_fmt_desc_get(st->codecpar->format);
+    av_assert0(desc);
+    is_rgb = desc->flags & AV_PIX_FMT_FLAG_RGB;
+
+    pos = mxf_write_cdci_common(s, st, is_rgb ? mxf_rgba_descriptor_key : mxf_cdci_descriptor_key);
+    mxf_update_klv_size(s->pb, pos);
+    mxf_write_ffv1_subdesc(s, st);
+}
+
 static void mxf_write_s436m_anc_desc(AVFormatContext *s, AVStream *st)
 {
     int64_t pos = mxf_write_generic_desc(s, st, mxf_s436m_anc_descriptor_key);
@@ -2347,6 +2434,73 @@ static int mxf_parse_h264_frame(AVFormatContext *s, AVStream *st,
     return 1;
 }
 
+static inline int get_ffv1_unsigned_symbol(RangeCoder *c, uint8_t *state) {
+    if(get_rac(c, state+0))
+        return 0;
+    else{
+        int i, e;
+        unsigned a;
+        e= 0;
+        while(get_rac(c, state+1 + FFMIN(e,9))){ //1..10
+            e++;
+            if (e > 31)
+                return AVERROR_INVALIDDATA;
+        }
+
+        a= 1;
+        for(i=e-1; i>=0; i--){
+            a += a + get_rac(c, state+22 + FFMIN(i,9)); //22..31
+        }
+
+        return a;
+    }
+}
+#define FFV1_CONTEXT_SIZE 32
+static int mxf_parse_ffv1_frame(AVFormatContext *s, AVStream *st, AVPacket *pkt)
+{
+    MXFContext *mxf = s->priv_data;
+    MXFStreamContext *sc = st->priv_data;
+    uint8_t state[FFV1_CONTEXT_SIZE];
+    RangeCoder c;
+    unsigned v;
+
+    sc->frame_size = pkt->size;
+
+    if (mxf->header_written)
+        return 1;
+
+    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);
+    }
+    sc->codec_ul = &mxf_ffv1_codec_uls[v];
+    
+    if (st->codecpar->field_order > AV_FIELD_PROGRESSIVE) {
+        sc->interlaced = 1;
+        sc->field_dominance = st->codecpar->field_order == (st->codecpar->field_order == AV_FIELD_TT || st->codecpar->field_order == AV_FIELD_TB) ? 1 : 2;
+    }
+    sc->aspect_ratio.num = st->codecpar->width * st->codecpar->sample_aspect_ratio.num;
+    sc->aspect_ratio.den = st->codecpar->height * st->codecpar->sample_aspect_ratio.den;
+    av_reduce(&sc->aspect_ratio.num, &sc->aspect_ratio.den,
+              sc->aspect_ratio.num, sc->aspect_ratio.den, INT_MAX);
+
+    return 1;
+}
+
 static const UID mxf_mpeg2_codec_uls[] = {
     { 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x03,0x04,0x01,0x02,0x02,0x01,0x01,0x10,0x00 }, // MP-ML I-Frame
     { 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x03,0x04,0x01,0x02,0x02,0x01,0x01,0x11,0x00 }, // MP-ML Long GOP
@@ -2958,6 +3112,11 @@ static int mxf_write_packet(AVFormatContext *s, AVPacket *pkt)
             av_log(s, AV_LOG_ERROR, "could not get h264 profile\n");
             return -1;
         }
+    } 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;
+        }
     }
 
     if (mxf->cbr_index) {
diff --git a/libavformat/rangecoder_dec.c b/libavformat/rangecoder_dec.c
new file mode 100644
index 0000000000..7b727f656e
--- /dev/null
+++ b/libavformat/rangecoder_dec.c
@@ -0,0 +1 @@
+#include "libavcodec/rangecoder.c"
diff --git a/tests/fate/lavf-container.mak b/tests/fate/lavf-container.mak
index 4bf7636b56..214ad6fc0d 100644
--- a/tests/fate/lavf-container.mak
+++ b/tests/fate/lavf-container.mak
@@ -8,6 +8,7 @@ FATE_LAVF_CONTAINER-$(call ENCDEC2, MPEG4,      MP2,       MATROSKA)           +
 FATE_LAVF_CONTAINER-$(call ENCDEC2, MPEG4,      PCM_ALAW,  MOV)                += mov mov_rtphint ismv
 FATE_LAVF_CONTAINER-$(call ENCDEC,  MPEG4,                 MOV)                += mp4
 FATE_LAVF_CONTAINER-$(call ENCDEC2, MPEG1VIDEO, MP2,       MPEG1SYSTEM MPEGPS) += mpg
+FATE_LAVF_CONTAINER-$(call ENCDEC , FFV1,                  MXF)                += mxf_ffv1
 FATE_LAVF_CONTAINER-$(call ENCDEC2, MPEG2VIDEO, PCM_S16LE, MXF)                += mxf mxf_dv25 mxf_dvcpro50
 FATE_LAVF_CONTAINER-$(call ENCDEC2, MPEG2VIDEO, PCM_S16LE, MXF_D10 MXF)        += mxf_d10
 FATE_LAVF_CONTAINER-$(call ENCDEC2, DNXHD,      PCM_S16LE, MXF_OPATOM MXF)     += mxf_opatom mxf_opatom_audio
@@ -55,6 +56,7 @@ fate-lavf-mxf: CMD = lavf_container_timecode "-ar 48000 -bf 2 -threads 1"
 fate-lavf-mxf_d10: CMD = lavf_container "-ar 48000 -ac 2" "-r 25 -vf scale=720:576,pad=720:608:0:32 -c:v mpeg2video -g 0 -flags +ildct+low_delay -dc 10 -non_linear_quant 1 -intra_vlc 1 -qscale 1 -ps 1 -qmin 1 -rc_max_vbv_use 1 -rc_min_vbv_use 1 -pix_fmt yuv422p -minrate 30000k -maxrate 30000k -b 30000k -bufsize 1200000 -top 1 -rc_init_occupancy 1200000 -qmax 12 -f mxf_d10"
 fate-lavf-mxf_dv25: CMD = lavf_container "-ar 48000 -ac 2" "-r 25 -vf scale=720:576,setdar=4/3 -c:v dvvideo -pix_fmt yuv420p -b 25000k -top 0 -f mxf"
 fate-lavf-mxf_dvcpro50: CMD = lavf_container "-ar 48000 -ac 2" "-r 25 -vf scale=720:576,setdar=16/9 -c:v dvvideo -pix_fmt yuv422p -b 50000k -top 0 -f mxf"
+fate-lavf-mxf_ffv1: CMD = lavf_container "-an" "-r 25 -vf scale=720:576,setdar=4/3 -c:v ffv1 -level 3 -pix_fmt yuv420p -f mxf"
 fate-lavf-mxf_opatom: CMD = lavf_container "" "-s 1920x1080 -c:v dnxhd -pix_fmt yuv422p -vb 36M -f mxf_opatom -map 0"
 fate-lavf-mxf_opatom_audio: CMD = lavf_container "-ar 48000 -ac 1" "-f mxf_opatom -mxf_audio_edit_rate 25 -map 1"
 fate-lavf-smjpeg:  CMD = lavf_container "" "-f smjpeg"
diff --git a/tests/ref/lavf/mxf_ffv1 b/tests/ref/lavf/mxf_ffv1
new file mode 100644
index 0000000000..b89e6a214e
--- /dev/null
+++ b/tests/ref/lavf/mxf_ffv1
@@ -0,0 +1,3 @@
+0426b93bbf06aedf083f00cd4ebaf36f *tests/data/lavf/lavf.mxf_ffv1
+5587001 tests/data/lavf/lavf.mxf_ffv1
+tests/data/lavf/lavf.mxf_ffv1 CRC=0x02354cdc
-- 
2.13.3.windows.1


[-- Attachment #3: Type: text/plain, Size: 251 bytes --]

_______________________________________________
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] avformat/mxfenc: SMPTE RDD 48:2018 Amd 1:2022 (FFV1 in MXF) support

[Tomas Härdin]

tis 2023-03-14 klockan 10:52 +0100 skrev Jerome Martinez:
> On 31/01/2023 15:53, Tomas Härdin wrote:
> > sön 2023-01-29 klockan 11:36 -0500 skrev Dave Rice:
> > > 
> > > I'm just nudging on the consideration of merging this patch. I've
> > > been testing it over the last week with ffv1/mxf content and have
> > > found this demuxing support very helpful.
> > Surely you mean muxing?
> > 
> > Some FATE tests would be nice.
> 
> 
> Apologizes for the huge delay.
> Attached is an updated patch with a FATE test.

Test passes. Maybe someone who knows more about FFV1 wants to chime in?

/Tomas

_______________________________________________
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] avformat/mxfenc: SMPTE RDD 48:2018 Amd 1:2022 (FFV1 in MXF) support

[Dave Rice]

> On Mar 15, 2023, at 10:00 AM, Tomas Härdin <git@haerdin.se> wrote:
> 
> tis 2023-03-14 klockan 10:52 +0100 skrev Jerome Martinez:
>> On 31/01/2023 15:53, Tomas Härdin wrote:
>>> sön 2023-01-29 klockan 11:36 -0500 skrev Dave Rice:
>>>> 
>>>> I'm just nudging on the consideration of merging this patch. I've
>>>> been testing it over the last week with ffv1/mxf content and have
>>>> found this demuxing support very helpful.
>>> Surely you mean muxing?
>>> 
>>> Some FATE tests would be nice.
>> 
>> 
>> Apologizes for the huge delay.
>> Attached is an updated patch with a FATE test.
> 
> Test passes. Maybe someone who knows more about FFV1 wants to chime in?

I can add that in the development of this patch, a contributor to SMPTE RDD 48:2018 Amd 1:2022 reviewed and approved the output ffv1/mxf files of this patch.
Kind Regards,
Dave Rice
_______________________________________________
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] avformat/mxfenc: SMPTE RDD 48:2018 Amd 1:2022 (FFV1 in MXF) support

[Tomas Härdin]

fre 2023-03-24 klockan 09:59 -0400 skrev Dave Rice:
> 
> > On Mar 15, 2023, at 10:00 AM, Tomas Härdin <git@haerdin.se> wrote:
> > 
> > tis 2023-03-14 klockan 10:52 +0100 skrev Jerome Martinez:
> > > On 31/01/2023 15:53, Tomas Härdin wrote:
> > > > sön 2023-01-29 klockan 11:36 -0500 skrev Dave Rice:
> > > > > 
> > > > > I'm just nudging on the consideration of merging this patch.
> > > > > I've
> > > > > been testing it over the last week with ffv1/mxf content and
> > > > > have
> > > > > found this demuxing support very helpful.
> > > > Surely you mean muxing?
> > > > 
> > > > Some FATE tests would be nice.
> > > 
> > > 
> > > Apologizes for the huge delay.
> > > Attached is an updated patch with a FATE test.
> > 
> > Test passes. Maybe someone who knows more about FFV1 wants to chime
> > in?
> 
> I can add that in the development of this patch, a contributor to
> SMPTE RDD 48:2018 Amd 1:2022 reviewed and approved the output
> ffv1/mxf files of this patch.

Good enough. Pushed. I fixed a whitespace issue while I was at it.

/Tomas

_______________________________________________
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] avformat/mxfenc: SMPTE RDD 48:2018 Amd 1:2022 (FFV1 in MXF) support

[Michael Niedermayer]

[-- Attachment #1.1: Type: text/plain, Size: 19343 bytes --]

On Tue, Mar 14, 2023 at 10:52:15AM +0100, Jerome Martinez wrote:
> On 31/01/2023 15:53, Tomas Härdin wrote:
> > sön 2023-01-29 klockan 11:36 -0500 skrev Dave Rice:
> > > 
> > > > On Jan 20, 2023, at 10:17 AM, Tomas Härdin <git@haerdin.se> wrote:
> > > > 
> > > > ons 2023-01-18 klockan 15:15 +0100 skrev Jerome Martinez:
> > > > > On 18/01/2023 14:40, Tomas Härdin wrote:
> > > > > > Creating a new subthread because I just noticed something
> > > > > I am a bit lost there because the line of code below is not part
> > > > > of
> > > > > this
> > > > > FFV1 patch.
> > > > > Additionally, none on my patches (FFV1 of MXF
> > > > > stored/sampled/displayed
> > > > > fix) modifies the discussed behavior (FFmpeg behavior would be
> > > > > same
> > > > > before and after this patch for MPEG-2 and AVC), so should not
> > > > > block
> > > > > any
> > > > > of them, and a potential fix for that should have its own patch
> > > > > as it
> > > > > would be a separate issue.
> > > > True, it doesn't need to hold up this patch. But some discussion is
> > > > warranted I think. I might create a separate patchset for this.
> > > > 
> > > > > Anyway:
> > > > > 
> > > > > 
> > > > > > > +    //Stored height
> > > > > > >        mxf_write_local_tag(s, 4, 0x3202);
> > > > > > >        avio_wb32(pb, stored_height>>sc->interlaced);
> > > > > > > 
> > > > > > Won't this be incorrect for files whose dimensions are
> > > > > > multiples of
> > > > > > 16
> > > > > > but not multiples of 32? Isn't each field stored separately
> > > > > > with
> > > > > > dimensions a multiple of 16? So while for 1080p we'll have
> > > > > > 
> > > > > >     StoredHeight = 1088
> > > > > >     SampledHeight = 1080
> > > > > > 
> > > > > > and 1080i:
> > > > > > 
> > > > > >     StoredHeight = 544
> > > > > >     SampledHeight = 540
> > > > > > 
> > > > > > Where 544 is a multiple of 16, for say 720p we have
> > > > > > 
> > > > > >     StoredHeight = 720
> > > > > >     SampledHeight = 720
> > > > > > 
> > > > > > but for a hypothetical 720i we'd get
> > > > > > 
> > > > > >     StoredHeight = 360
> > > > > >     SampledHeight = 360
> > > > > > 
> > > > > > whereas the correct values should be
> > > > > > 
> > > > > >     StoredHeight = 368
> > > > > >     SampledHeight = 360
> > > > > AFAIK, it would depend about if the stream has a
> > > > > picture_structure
> > > > > frame
> > > > > (16x16 applies to the frame?) of field (16x16 applies to the
> > > > > field?),
> > > > > but I really don't know enough there for having a relevant
> > > > > opinion.
> > > > > 
> > > > > I can just say that I don't change the behavior of FFmpeg in your
> > > > > use
> > > > > case, I found the issues when I tried a random width and height
> > > > > of
> > > > > FFV1
> > > > > stream then checked with MPEG-2 Video and the sampled width was
> > > > > wrong
> > > > > for sure e.g. sampled width of 1920 for a stream having a width
> > > > > of
> > > > > 1912,
> > > > > with current FFmpeg code, and for your use case I am sure about
> > > > > nothing
> > > > > so I don't change the behavior with my patch, IMO if there is an
> > > > > issue
> > > > > with 720i MPEG-2 Video it should be in a separate topic and patch
> > > > > as
> > > > > it
> > > > > would modify the "stored_height = (st->codecpar-
> > > > > > height+15)/16*16"
> > > > > current code (in my patch I just move this code), unless we are
> > > > > sure
> > > > > of
> > > > > what should be changed on this side and apply a fix on the way.
> > > > > Better
> > > > > to fix 1 issue and let 1 open with no change than fixing no issue
> > > > > because we wouldn't be sure for 1 of the 2.
> > > > I suspect we are lucky because 720i doesn't really exist in the
> > > > real
> > > > world, and 576i and 480i are both multiples of 32.
> > > > 
> > > > IMO mxfenc shouldn't lie, but looking at S377m StoredWidth/Height
> > > > are
> > > > "best effort" and thus shall be encoded. Their values will depend
> > > > on
> > > > FrameLayout which in turn depends on what you say - how exactly the
> > > > interlacing is done.
> > > > 
> > > > TL;DR: this patchset doesn't need to be held up by this.
> > > I'm just nudging on the consideration of merging this patch. I've
> > > been testing it over the last week with ffv1/mxf content and have
> > > found this demuxing support very helpful.
> > Surely you mean muxing?
> > 
> > Some FATE tests would be nice.
> 
> 
> Apologizes for the huge delay.
> Attached is an updated patch with a FATE test.
> 
> Jérôme

>  libavformat/Makefile          |    3 
>  libavformat/mxfenc.c          |  163 +++++++++++++++++++++++++++++++++++++++++-
>  libavformat/rangecoder_dec.c  |    1 
>  tests/fate/lavf-container.mak |    2 
>  tests/ref/lavf/mxf_ffv1       |    3 
>  5 files changed, 169 insertions(+), 3 deletions(-)
> 1135a876a0c3b46caf20af8823da50e7b62e0fb2  0001-avformat-mxfenc-SMPTE-RDD-48-2018-Amd-1-2022-support.patch
> From fec4b918dd6e6a067eaeb2cd27f5e42c08dcca2c Mon Sep 17 00:00:00 2001
> From: Jerome Martinez <jerome@mediaarea.net>
> Date: Tue, 14 Mar 2023 09:49:16 +0100
> Subject: [PATCH] avformat/mxfenc: SMPTE RDD 48:2018 Amd 1:2022 support
> 
> ---
>  libavformat/Makefile          |   3 +-
>  libavformat/mxfenc.c          | 163 +++++++++++++++++++++++++++++++++++++++++-
>  libavformat/rangecoder_dec.c  |   1 +
>  tests/fate/lavf-container.mak |   2 +
>  tests/ref/lavf/mxf_ffv1       |   3 +
>  5 files changed, 169 insertions(+), 3 deletions(-)
>  create mode 100644 libavformat/rangecoder_dec.c
>  create mode 100644 tests/ref/lavf/mxf_ffv1
> 
> diff --git a/libavformat/Makefile b/libavformat/Makefile
> index 47bbbbfb2a..048649689b 100644
> --- a/libavformat/Makefile
> +++ b/libavformat/Makefile
> @@ -712,7 +712,8 @@ SHLIBOBJS-$(CONFIG_HLS_DEMUXER)          += ac3_channel_layout_tab.o
>  SHLIBOBJS-$(CONFIG_MATROSKA_DEMUXER)     += mpeg4audio_sample_rates.o
>  SHLIBOBJS-$(CONFIG_MOV_DEMUXER)          += ac3_channel_layout_tab.o
>  SHLIBOBJS-$(CONFIG_MP3_MUXER)            += mpegaudiotabs.o
> -SHLIBOBJS-$(CONFIG_MXF_MUXER)            += golomb_tab.o
> +SHLIBOBJS-$(CONFIG_MXF_MUXER)            += golomb_tab.o \
> +                                            rangecoder_dec.o
>  SHLIBOBJS-$(CONFIG_NUT_MUXER)            += mpegaudiotabs.o
>  SHLIBOBJS-$(CONFIG_RTPDEC)               += jpegtables.o
>  SHLIBOBJS-$(CONFIG_RTP_MUXER)            += golomb_tab.o jpegtables.o \
> diff --git a/libavformat/mxfenc.c b/libavformat/mxfenc.c
> index a29d678098..0ff566fbb4 100644
> --- a/libavformat/mxfenc.c
> +++ b/libavformat/mxfenc.c
> @@ -51,6 +51,7 @@
>  #include "libavcodec/golomb.h"
>  #include "libavcodec/h264.h"
>  #include "libavcodec/packet_internal.h"
> +#include "libavcodec/rangecoder.h"
>  #include "libavcodec/startcode.h"
>  #include "avformat.h"
>  #include "avio_internal.h"
> @@ -102,6 +103,7 @@ typedef struct MXFStreamContext {
>      int b_picture_count;     ///< maximum number of consecutive b pictures, used in mpeg-2 descriptor
>      int low_delay;           ///< low delay, used in mpeg-2 descriptor
>      int avc_intra;
> +    int micro_version;       ///< format micro_version, used in ffv1 descriptor
>  } MXFStreamContext;
>  
>  typedef struct MXFContainerEssenceEntry {
> @@ -130,6 +132,7 @@ enum ULIndex {
>      INDEX_H264,
>      INDEX_S436M,
>      INDEX_PRORES,
> +    INDEX_FFV1,
>  };
>  
>  static const struct {
> @@ -144,6 +147,7 @@ static const struct {
>      { AV_CODEC_ID_JPEG2000,   INDEX_JPEG2000 },
>      { AV_CODEC_ID_H264,       INDEX_H264 },
>      { AV_CODEC_ID_PRORES,     INDEX_PRORES },
> +    { AV_CODEC_ID_FFV1,       INDEX_FFV1 },
>      { AV_CODEC_ID_NONE }
>  };
>  
> @@ -151,6 +155,7 @@ static void mxf_write_wav_desc(AVFormatContext *s, AVStream *st);
>  static void mxf_write_aes3_desc(AVFormatContext *s, AVStream *st);
>  static void mxf_write_mpegvideo_desc(AVFormatContext *s, AVStream *st);
>  static void mxf_write_h264_desc(AVFormatContext *s, AVStream *st);
> +static void mxf_write_ffv1_desc(AVFormatContext *s, AVStream *st);
>  static void mxf_write_cdci_desc(AVFormatContext *s, AVStream *st);
>  static void mxf_write_generic_sound_desc(AVFormatContext *s, AVStream *st);
>  static void mxf_write_s436m_anc_desc(AVFormatContext *s, AVStream *st);
> @@ -208,6 +213,11 @@ static const MXFContainerEssenceEntry mxf_essence_container_uls[] = {
>        { 0x06,0x0E,0x2B,0x34,0x01,0x02,0x01,0x01,0x0d,0x01,0x03,0x01,0x15,0x01,0x17,0x00 },
>        { 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x0d,0x04,0x01,0x02,0x02,0x03,0x06,0x03,0x00 },
>        mxf_write_cdci_desc },
> +    // FFV1
> +    { { 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x0d,0x0d,0x01,0x03,0x01,0x02,0x23,0x01,0x00 },
> +      { 0x06,0x0E,0x2B,0x34,0x01,0x02,0x01,0x01,0x0d,0x01,0x03,0x01,0x15,0x01,0x1d,0x00 },
> +      { 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x0d,0x04,0x01,0x02,0x02,0x03,0x09,0x00,0x00 },
> +      mxf_write_ffv1_desc },
>      { { 0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00 },
>        { 0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00 },
>        { 0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00 },
> @@ -232,6 +242,15 @@ static const UID mxf_d10_container_uls[] = {
>      { 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x01,0x0D,0x01,0x03,0x01,0x02,0x01,0x06,0x01 }, // D-10 525/50 NTSC 30mb/s
>  };
>  
> +
> +static const UID mxf_ffv1_codec_uls[] = {
> +    { 0x06,0x0e,0x2b,0x34,0x04,0x01,0x01,0x0D,0x04,0x01,0x02,0x02,0x03,0x09,0x01,0x00 }, // FFV1 version 0
> +    { 0x06,0x0e,0x2b,0x34,0x04,0x01,0x01,0x0D,0x04,0x01,0x02,0x02,0x03,0x09,0x02,0x00 }, // FFV1 version 1
> +    { 0x06,0x0e,0x2b,0x34,0x04,0x01,0x01,0x0D,0x04,0x01,0x02,0x02,0x03,0x09,0x03,0x00 }, // FFV1 version 2 (was only experimental)
> +    { 0x06,0x0e,0x2b,0x34,0x04,0x01,0x01,0x0D,0x04,0x01,0x02,0x02,0x03,0x09,0x04,0x00 }, // FFV1 version 3
> +    { 0x06,0x0e,0x2b,0x34,0x04,0x01,0x01,0x0D,0x04,0x01,0x02,0x02,0x03,0x09,0x05,0x00 }, // FFV1 version 4
> +};
> +
>  static const uint8_t product_uid[]          = { 0xAD,0xAB,0x44,0x24,0x2f,0x25,0x4d,0xc7,0x92,0xff,0x29,0xbd,0x00,0x0c,0x00,0x02};
>  static const uint8_t uuid_base[]            = { 0xAD,0xAB,0x44,0x24,0x2f,0x25,0x4d,0xc7,0x92,0xff };
>  static const uint8_t umid_ul[]              = { 0x06,0x0A,0x2B,0x34,0x01,0x01,0x01,0x05,0x01,0x01,0x0D,0x00,0x13 };
> @@ -390,6 +409,10 @@ static const MXFLocalTagPair mxf_local_tag_batch[] = {
>      { 0x8302, FF_MXF_MasteringDisplayWhitePointChromaticity },
>      { 0x8303, FF_MXF_MasteringDisplayMaximumLuminance },
>      { 0x8304, FF_MXF_MasteringDisplayMinimumLuminance },
> +    // FFV1
> +    { 0xDFD9, {0x06,0x0E,0x2B,0x34,0x01,0x01,0x01,0x0E,0x04,0x01,0x06,0x0C,0x06,0x00,0x00,0x00}}, /* FFV1 Micro-version */
> +    { 0xDFDA, {0x06,0x0E,0x2B,0x34,0x01,0x01,0x01,0x0E,0x04,0x01,0x06,0x0C,0x05,0x00,0x00,0x00}}, /* FFV1 Version */
> +    { 0xDFDB, {0x06,0x0E,0x2B,0x34,0x01,0x01,0x01,0x0E,0x04,0x01,0x06,0x0C,0x01,0x00,0x00,0x00}}, /* FFV1 Initialization Metadata */
>  };
>  
>  #define MXF_NUM_TAGS FF_ARRAY_ELEMS(mxf_local_tag_batch)
> @@ -526,7 +549,7 @@ static void mxf_write_primer_pack(AVFormatContext *s)
>      MXFContext *mxf = s->priv_data;
>      AVIOContext *pb = s->pb;
>      int local_tag_number = MXF_NUM_TAGS, i;
> -    int will_have_avc_tags = 0, will_have_mastering_tags = 0;
> +    int will_have_avc_tags = 0, will_have_mastering_tags = 0, will_have_ffv1_tags = 0;
>  
>      for (i = 0; i < s->nb_streams; i++) {
>          MXFStreamContext *sc = s->streams[i]->priv_data;
> @@ -536,6 +559,9 @@ static void mxf_write_primer_pack(AVFormatContext *s)
>          if (av_stream_get_side_data(s->streams[i], AV_PKT_DATA_MASTERING_DISPLAY_METADATA, NULL)) {
>              will_have_mastering_tags = 1;
>          }
> +        if (s->streams[i]->codecpar->codec_id == AV_CODEC_ID_FFV1) {
> +            will_have_ffv1_tags = 1;
> +        }
>      }
>  
>      if (!mxf->store_user_comments) {
> @@ -544,8 +570,11 @@ static void mxf_write_primer_pack(AVFormatContext *s)
>          mxf_mark_tag_unused(mxf, 0x5003);
>      }
>  
> -    if (!will_have_avc_tags) {
> +    if (!will_have_avc_tags && !will_have_ffv1_tags) {
>          mxf_mark_tag_unused(mxf, 0x8100);
> +    }
> +
> +    if (!will_have_avc_tags) {
>          mxf_mark_tag_unused(mxf, 0x8200);
>          mxf_mark_tag_unused(mxf, 0x8201);
>          mxf_mark_tag_unused(mxf, 0x8202);
> @@ -558,6 +587,12 @@ static void mxf_write_primer_pack(AVFormatContext *s)
>          mxf_mark_tag_unused(mxf, 0x8304);
>      }
>  
> +    if (!will_have_ffv1_tags) {
> +        mxf_mark_tag_unused(mxf, 0xDFD9);
> +        mxf_mark_tag_unused(mxf, 0xDFDA);
> +        mxf_mark_tag_unused(mxf, 0xDFDB);
> +    }
> +
>      for (i = 0; i < MXF_NUM_TAGS; i++) {
>          if (mxf->unused_tags[i]) {
>              local_tag_number--;
> @@ -1094,9 +1129,11 @@ static const UID mxf_mpegvideo_descriptor_key = { 0x06,0x0E,0x2B,0x34,0x02,0x53,
>  static const UID mxf_wav_descriptor_key       = { 0x06,0x0E,0x2B,0x34,0x02,0x53,0x01,0x01,0x0d,0x01,0x01,0x01,0x01,0x01,0x48,0x00 };
>  static const UID mxf_aes3_descriptor_key      = { 0x06,0x0E,0x2B,0x34,0x02,0x53,0x01,0x01,0x0d,0x01,0x01,0x01,0x01,0x01,0x47,0x00 };
>  static const UID mxf_cdci_descriptor_key      = { 0x06,0x0E,0x2B,0x34,0x02,0x53,0x01,0x01,0x0D,0x01,0x01,0x01,0x01,0x01,0x28,0x00 };
> +static const UID mxf_rgba_descriptor_key      = { 0x06,0x0E,0x2B,0x34,0x02,0x53,0x01,0x01,0x0D,0x01,0x01,0x01,0x01,0x01,0x29,0x00 };
>  static const UID mxf_generic_sound_descriptor_key = { 0x06,0x0E,0x2B,0x34,0x02,0x53,0x01,0x01,0x0D,0x01,0x01,0x01,0x01,0x01,0x42,0x00 };
>  
>  static const UID mxf_avc_subdescriptor_key = { 0x06,0x0E,0x2B,0x34,0x02,0x53,0x01,0x01,0x0d,0x01,0x01,0x01,0x01,0x01,0x6E,0x00 };
> +static const UID mxf_ffv1_subdescriptor_key = { 0x06,0x0E,0x2B,0x34,0x02,0x53,0x01,0x01,0x0d,0x01,0x01,0x01,0x01,0x01,0x81,0x03 };
>  
>  static inline uint16_t rescale_mastering_chroma(AVRational q)
>  {
> @@ -1198,6 +1235,7 @@ static int64_t mxf_write_cdci_common(AVFormatContext *s, AVStream *st, const UID
>          avio_wb32(pb, -((st->codecpar->height - display_height)&1));
>      }
>  
> +    if (key != mxf_rgba_descriptor_key) {
>      // component depth
>      mxf_write_local_tag(s, 4, 0x3301);
>      avio_wb32(pb, sc->component_depth);
> @@ -1234,6 +1272,7 @@ static int64_t mxf_write_cdci_common(AVFormatContext *s, AVStream *st, const UID
>          mxf_write_local_tag(s, 4, 0x3306);
>          avio_wb32(pb, color);
>      }
> +    } // if (key != mxf_rgba_descriptor_key)
>  
>      if (sc->signal_standard) {
>          mxf_write_local_tag(s, 1, 0x3215);
> @@ -1329,6 +1368,13 @@ static int64_t mxf_write_cdci_common(AVFormatContext *s, AVStream *st, const UID
>          mxf_write_uuid(pb, AVCSubDescriptor, 0);
>      }
>  
> +    if (st->codecpar->codec_id == AV_CODEC_ID_FFV1) {
> +        // write ffv1 sub descriptor ref
> +        mxf_write_local_tag(s, 8 + 16, 0x8100);
> +        mxf_write_refs_count(pb, 1);
> +        mxf_write_uuid(pb, FFV1SubDescriptor, 0);
> +    }
> +
>      return pos;
>  }
>  
> @@ -1387,6 +1433,47 @@ static void mxf_write_h264_desc(AVFormatContext *s, AVStream *st)
>      }
>  }
>  
> +static void mxf_write_ffv1_subdesc(AVFormatContext *s, AVStream *st)
> +{
> +    AVIOContext *pb = s->pb;
> +    MXFStreamContext *sc = st->priv_data;
> +    int64_t pos;
> +
> +    avio_write(pb, mxf_ffv1_subdescriptor_key, 16);
> +    klv_encode_ber4_length(pb, 0);
> +    pos = avio_tell(pb);
> +    
> +    mxf_write_local_tag(s, 16, 0x3C0A);
> +    mxf_write_uuid(pb, FFV1SubDescriptor, 0);
> +
> +    if (st->codecpar->extradata_size) {
> +        mxf_write_local_tag(s, st->codecpar->extradata_size, 0xDFDB);
> +        avio_write(pb, st->codecpar->extradata, st->codecpar->extradata_size); // FFV1InitializationMetadata
> +    }
> +
> +    mxf_write_local_tag(s, 2, 0xDFDA);
> +    avio_wb16(pb, (*sc->codec_ul)[14]); // FFV1Version
> +
> +    if (st->codecpar->extradata_size) {
> +        mxf_write_local_tag(s, 2, 0xDFD9);
> +        avio_wb16(pb, sc->micro_version); // FFV1MicroVersion
> +    }
> +
> +    mxf_update_klv_size(s->pb, pos);
> +}
> +
> +static void mxf_write_ffv1_desc(AVFormatContext *s, AVStream *st)
> +{
> +    int is_rgb, pos;
> +    const AVPixFmtDescriptor *desc = av_pix_fmt_desc_get(st->codecpar->format);
> +    av_assert0(desc);
> +    is_rgb = desc->flags & AV_PIX_FMT_FLAG_RGB;
> +
> +    pos = mxf_write_cdci_common(s, st, is_rgb ? mxf_rgba_descriptor_key : mxf_cdci_descriptor_key);
> +    mxf_update_klv_size(s->pb, pos);
> +    mxf_write_ffv1_subdesc(s, st);
> +}
> +
>  static void mxf_write_s436m_anc_desc(AVFormatContext *s, AVStream *st)
>  {
>      int64_t pos = mxf_write_generic_desc(s, st, mxf_s436m_anc_descriptor_key);
> @@ -2347,6 +2434,73 @@ static int mxf_parse_h264_frame(AVFormatContext *s, AVStream *st,
>      return 1;
>  }
>  
> +static inline int get_ffv1_unsigned_symbol(RangeCoder *c, uint8_t *state) {
> +    if(get_rac(c, state+0))
> +        return 0;
> +    else{
> +        int i, e;
> +        unsigned a;
> +        e= 0;
> +        while(get_rac(c, state+1 + FFMIN(e,9))){ //1..10
> +            e++;
> +            if (e > 31)
> +                return AVERROR_INVALIDDATA;
> +        }
> +
> +        a= 1;
> +        for(i=e-1; i>=0; i--){
> +            a += a + get_rac(c, state+22 + FFMIN(i,9)); //22..31
> +        }
> +
> +        return a;
> +    }
> +}
> +#define FFV1_CONTEXT_SIZE 32
> +static int mxf_parse_ffv1_frame(AVFormatContext *s, AVStream *st, AVPacket *pkt)
> +{
> +    MXFContext *mxf = s->priv_data;
> +    MXFStreamContext *sc = st->priv_data;
> +    uint8_t state[FFV1_CONTEXT_SIZE];
> +    RangeCoder c;
> +    unsigned v;
> +
> +    sc->frame_size = pkt->size;
> +
> +    if (mxf->header_written)
> +        return 1;
> +
> +    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 ?

thx

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

The greatest way to live with honor in this world is to be what we pretend
to be. -- Socrates

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
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] avformat/mxfenc: SMPTE RDD 48:2018 Amd 1:2022 (FFV1 in MXF) support

[Jerome Martinez]

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.

_______________________________________________
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] avformat/mxfenc: SMPTE RDD 48:2018 Amd 1:2022 (FFV1 in MXF) support

[Michael Niedermayer]

[-- Attachment #1.1: Type: text/plain, Size: 1865 bytes --]

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

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Never trust a computer, one day, it may think you are the virus. -- Compn

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
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] avformat/mxfenc: SMPTE RDD 48:2018 Amd 1:2022 (FFV1 in MXF) support

[Jerome Martinez]

[-- Attachment #1: Type: text/plain, Size: 2739 bytes --]

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.

[-- Attachment #2: 0001-avformat-mxfenc-replace-ffv1-version-related-assert-.patch --]
[-- Type: text/plain, Size: 1870 bytes --]

From 65f6fba9434fa9ab4b5d2b91c3f930c1e3cb4bef Mon Sep 17 00:00:00 2001
From: Jerome Martinez <jerome@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


[-- Attachment #3: Type: text/plain, Size: 251 bytes --]

_______________________________________________
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] avformat/mxfenc: SMPTE RDD 48:2018 Amd 1:2022 (FFV1 in MXF) support

[Michael Niedermayer]

[-- Attachment #1.1: Type: text/plain, Size: 4541 bytes --]

On Sat, Apr 01, 2023 at 07:11:45PM +0200, Jerome Martinez wrote:
> 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.

>  mxfenc.c |   10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 2b3a2cc619fcaf2dd1da7aa4b566b390eced62a3  0001-avformat-mxfenc-replace-ffv1-version-related-assert-.patch
> From 65f6fba9434fa9ab4b5d2b91c3f930c1e3cb4bef Mon Sep 17 00:00:00 2001
> From: Jerome Martinez <jerome@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;
>          }

should this not also fail with v > 4 as before ?

thx

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

If you think the mosad wants you dead since a long time then you are either
wrong or dead since a long time.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
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] avformat/mxfenc: SMPTE RDD 48:2018 Amd 1:2022 (FFV1 in MXF) support

[Jerome Martinez]

[-- Attachment #1: Type: text/plain, Size: 3473 bytes --]

On 02/04/2023 22:07, Michael Niedermayer wrote:
> On Sat, Apr 01, 2023 at 07:11:45PM +0200, Jerome Martinez wrote:
> [...]
>>   mxfenc.c |   10 ++++++----
>>   1 file changed, 6 insertions(+), 4 deletions(-)
>> 2b3a2cc619fcaf2dd1da7aa4b566b390eced62a3  0001-avformat-mxfenc-replace-ffv1-version-related-assert-.patch
>>  From 65f6fba9434fa9ab4b5d2b91c3f930c1e3cb4bef Mon Sep 17 00:00:00 2001
>> From: Jerome Martinez <jerome@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;
>>           }
> should this not also fail with v > 4 as before ?

I preferred during the change to copy the code in ffv1dec.c for more 
coherency, and ffv1dec.c considers version 5 in ver01 header as invalid 
and version 5 with extra metadata as legit.
Actually, a bigger issue is that ffv1dec.c accepts version 5 as if it 
can decode it, e.g. with hacked ffv1 stream having version 5:

$ ./ffmpeg -hide_banner -loglevel error -i level5_like_level1.mkv -f 
null /dev/null 2> >(grep -i  "version")
[ffv1 @ 0x7fffc04c81c0] invalid version 5 in ver01 header
$ mediainfo --Details=1 level5_like_level3.mkv | grep "version"
1B1        version:                         5 (0x00000005) - 
Error=FFV1-HEADER-version-LATERVERSION:1
$ ./ffmpeg -hide_banner -loglevel error -i level5_like_level3.mkv -f 
null /dev/null && echo $?
0

It works only because my hacked file is a version 4 in reality but it 
should be rejected for when we have a version 5 not compatible with 
version 4.
I suggest to keep the previous patch as is for coherency with ffv1dec.c 
then add the new attached patch that add a check on ffv1 version > 4 for 
both ffv1dec.c and mxfenc.c; for ffv1dec.c I also check micro_version 
for version 4 because FFV1 spec hints that micro_version before the one 
flagged as stable may be incompatible with previous micro_versions.
With the additional patch:
$ ./ffmpeg -hide_banner -loglevel error -i level5_like_level3.mkv -f 
null /dev/null
[ffv1 @ 0x7fffc290e640] unsupported version 5
Error while opening decoder for input stream #0:0 : Not yet implemented 
in FFmpeg, patches welcome
$ ./ffmpeg -hide_banner -loglevel error -i level4.3.mkv -f null /dev/null
[ffv1 @ 0x7fffc5b27640] unsupported version 4 micro_version 3
Error while opening decoder for input stream #0:0 : Not yet implemented 
in FFmpeg, patches welcome
$ ./ffmpeg -hide_banner -loglevel error -i level5_like_level3.mkv -r 25 
-c:v copy -f mxf /dev/null
[ffv1 @ 0x7fffe422bf00] unsupported version 5
[mxf @ 0x7fffda8b4080] unsupported ffv1 version 5
[out#0/mxf @ 0x7fffd34470c0] Error muxing a packet

Jérôme


[-- Attachment #2: 0001-avcodec-ffv1dec-reject-unsupported-ffv1-versions.patch --]
[-- Type: text/plain, Size: 1978 bytes --]

From 4434ebddb53524f52346b4fbb1126489d8864810 Mon Sep 17 00:00:00 2001
From: Jerome Martinez <jerome@mediaarea.net>
Date: Mon, 3 Apr 2023 00:04:53 +0200
Subject: [PATCH] avcodec/ffv1dec: reject unsupported ffv1 versions

And add similar check in libavformat/mxfenc
---
 libavcodec/ffv1dec.c | 10 ++++++++++
 libavformat/mxfenc.c |  4 ++++
 2 files changed, 14 insertions(+)

diff --git a/libavcodec/ffv1dec.c b/libavcodec/ffv1dec.c
index 180d24e695..7aaee7ec5e 100644
--- a/libavcodec/ffv1dec.c
+++ b/libavcodec/ffv1dec.c
@@ -439,11 +439,21 @@ static int read_extra_header(FFV1Context *f)
         av_log(f->avctx, AV_LOG_ERROR, "Invalid version in global header\n");
         return AVERROR_INVALIDDATA;
     }
+    if (f->version > 4) {
+        av_log(f->avctx, AV_LOG_ERROR, "unsupported version %d\n",
+            f->version);
+        return AVERROR_PATCHWELCOME;
+    }
     if (f->version > 2) {
         c->bytestream_end -= 4;
         f->micro_version = get_symbol(c, state, 0);
         if (f->micro_version < 0)
             return AVERROR_INVALIDDATA;
+        if (f->version == 4 && f->micro_version > 2) {
+            av_log(f->avctx, AV_LOG_ERROR, "unsupported version 4 micro_version %d\n",
+                f->micro_version);
+            return AVERROR_PATCHWELCOME;
+        }
     }
     f->ac = get_symbol(c, state, 0);
 
diff --git a/libavformat/mxfenc.c b/libavformat/mxfenc.c
index 0a1ae5353c..df5bb55ad8 100644
--- a/libavformat/mxfenc.c
+++ b/libavformat/mxfenc.c
@@ -2493,6 +2493,10 @@ static int mxf_parse_ffv1_frame(AVFormatContext *s, AVStream *st, AVPacket *pkt)
             av_log(s, AV_LOG_ERROR, "Invalid version in ffv1 global header\n");
             return 0;
         }
+        if (v > 4) {
+            av_log(s, AV_LOG_ERROR, "unsupported ffv1 version %d\n", v);
+            return 0;
+        }
         sc->micro_version = get_ffv1_unsigned_symbol(&c, state);
     } else {
         uint8_t keystate = 128;
-- 
2.13.3.windows.1


[-- Attachment #3: Type: text/plain, Size: 251 bytes --]

_______________________________________________
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] avformat/mxfenc: SMPTE RDD 48:2018 Amd 1:2022 (FFV1 in MXF) support

[Michael Niedermayer]

[-- Attachment #1.1: Type: text/plain, Size: 6358 bytes --]

On Mon, Apr 03, 2023 at 12:07:06AM +0200, Jerome Martinez wrote:
> On 02/04/2023 22:07, Michael Niedermayer wrote:
> > On Sat, Apr 01, 2023 at 07:11:45PM +0200, Jerome Martinez wrote:
> > [...]
> > >   mxfenc.c |   10 ++++++----
> > >   1 file changed, 6 insertions(+), 4 deletions(-)
> > > 2b3a2cc619fcaf2dd1da7aa4b566b390eced62a3  0001-avformat-mxfenc-replace-ffv1-version-related-assert-.patch
> > >  From 65f6fba9434fa9ab4b5d2b91c3f930c1e3cb4bef Mon Sep 17 00:00:00 2001
> > > From: Jerome Martinez <jerome@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;
> > >           }
> > should this not also fail with v > 4 as before ?
> 
> I preferred during the change to copy the code in ffv1dec.c for more
> coherency, and ffv1dec.c considers version 5 in ver01 header as invalid and
> version 5 with extra metadata as legit.
> Actually, a bigger issue is that ffv1dec.c accepts version 5 as if it can
> decode it, e.g. with hacked ffv1 stream having version 5:
> 
> $ ./ffmpeg -hide_banner -loglevel error -i level5_like_level1.mkv -f null
> /dev/null 2> >(grep -i  "version")
> [ffv1 @ 0x7fffc04c81c0] invalid version 5 in ver01 header
> $ mediainfo --Details=1 level5_like_level3.mkv | grep "version"
> 1B1        version:                         5 (0x00000005) -
> Error=FFV1-HEADER-version-LATERVERSION:1
> $ ./ffmpeg -hide_banner -loglevel error -i level5_like_level3.mkv -f null
> /dev/null && echo $?
> 0
> 
> It works only because my hacked file is a version 4 in reality but it should
> be rejected for when we have a version 5 not compatible with version 4.
> I suggest to keep the previous patch as is for coherency with ffv1dec.c then
> add the new attached patch that add a check on ffv1 version > 4 for both
> ffv1dec.c and mxfenc.c; for ffv1dec.c I also check micro_version for version
> 4 because FFV1 spec hints that micro_version before the one flagged as
> stable may be incompatible with previous micro_versions.
> With the additional patch:
> $ ./ffmpeg -hide_banner -loglevel error -i level5_like_level3.mkv -f null
> /dev/null
> [ffv1 @ 0x7fffc290e640] unsupported version 5
> Error while opening decoder for input stream #0:0 : Not yet implemented in
> FFmpeg, patches welcome
> $ ./ffmpeg -hide_banner -loglevel error -i level4.3.mkv -f null /dev/null
> [ffv1 @ 0x7fffc5b27640] unsupported version 4 micro_version 3
> Error while opening decoder for input stream #0:0 : Not yet implemented in
> FFmpeg, patches welcome
> $ ./ffmpeg -hide_banner -loglevel error -i level5_like_level3.mkv -r 25 -c:v
> copy -f mxf /dev/null
> [ffv1 @ 0x7fffe422bf00] unsupported version 5
> [mxf @ 0x7fffda8b4080] unsupported ffv1 version 5
> [out#0/mxf @ 0x7fffd34470c0] Error muxing a packet
> 
> Jérôme
> 

>  libavcodec/ffv1dec.c |   10 ++++++++++
>  libavformat/mxfenc.c |    4 ++++
>  2 files changed, 14 insertions(+)
> 14eb2ae57e1918a903d379640e466bdcb5a73a41  0001-avcodec-ffv1dec-reject-unsupported-ffv1-versions.patch
> From 4434ebddb53524f52346b4fbb1126489d8864810 Mon Sep 17 00:00:00 2001
> From: Jerome Martinez <jerome@mediaarea.net>
> Date: Mon, 3 Apr 2023 00:04:53 +0200
> Subject: [PATCH] avcodec/ffv1dec: reject unsupported ffv1 versions
> 
> And add similar check in libavformat/mxfenc
> ---
>  libavcodec/ffv1dec.c | 10 ++++++++++
>  libavformat/mxfenc.c |  4 ++++
>  2 files changed, 14 insertions(+)
> 
> diff --git a/libavcodec/ffv1dec.c b/libavcodec/ffv1dec.c
> index 180d24e695..7aaee7ec5e 100644
> --- a/libavcodec/ffv1dec.c
> +++ b/libavcodec/ffv1dec.c
> @@ -439,11 +439,21 @@ static int read_extra_header(FFV1Context *f)
>          av_log(f->avctx, AV_LOG_ERROR, "Invalid version in global header\n");
>          return AVERROR_INVALIDDATA;
>      }
> +    if (f->version > 4) {
> +        av_log(f->avctx, AV_LOG_ERROR, "unsupported version %d\n",
> +            f->version);
> +        return AVERROR_PATCHWELCOME;
> +    }
>      if (f->version > 2) {
>          c->bytestream_end -= 4;
>          f->micro_version = get_symbol(c, state, 0);
>          if (f->micro_version < 0)
>              return AVERROR_INVALIDDATA;
> +        if (f->version == 4 && f->micro_version > 2) {
> +            av_log(f->avctx, AV_LOG_ERROR, "unsupported version 4 micro_version %d\n",
> +                f->micro_version);
> +            return AVERROR_PATCHWELCOME;
> +        }
>      }
>      f->ac = get_symbol(c, state, 0);

you do not know if the decoder will have any problem with these files



>  
> diff --git a/libavformat/mxfenc.c b/libavformat/mxfenc.c
> index 0a1ae5353c..df5bb55ad8 100644
> --- a/libavformat/mxfenc.c
> +++ b/libavformat/mxfenc.c
> @@ -2493,6 +2493,10 @@ static int mxf_parse_ffv1_frame(AVFormatContext *s, AVStream *st, AVPacket *pkt)
>              av_log(s, AV_LOG_ERROR, "Invalid version in ffv1 global header\n");
>              return 0;
>          }
> +        if (v > 4) {
> +            av_log(s, AV_LOG_ERROR, "unsupported ffv1 version %d\n", v);
> +            return 0;
> +        }
>          sc->micro_version = get_ffv1_unsigned_symbol(&c, state);
>      } else {
>          uint8_t keystate = 128;

mxfenc change is ok

thx

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

The misfortune of the wise is better than the prosperity of the fool.
-- Epicurus

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
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] avformat/mxfenc: SMPTE RDD 48:2018 Amd 1:2022 (FFV1 in MXF) support

[Jerome Martinez]

[-- Attachment #1: Type: text/plain, Size: 878 bytes --]

On 04/04/2023 16:43, Michael Niedermayer wrote:
> On Mon, Apr 03, 2023 at 12:07:06AM +0200, Jerome Martinez wrote:
>> On 02/04/2023 22:07, Michael Niedermayer wrote:
>>
>> +        if (f->version == 4 && f->micro_version > 2) {
>> +            av_log(f->avctx, AV_LOG_ERROR, "unsupported version 4 micro_version %d\n",
>> +                f->micro_version);
>> +            return AVERROR_PATCHWELCOME;
>> +        }
>>       }
>>       f->ac = get_symbol(c, state, 0);
> you do not know if the decoder will have any problem with these files


But you don't don't if the decoder will have no problem, it seems safer 
to me to reject something we are not sure to support.
Anyway, I don't really care (the decoder will just fail later if it is 
not compatible + it is explicitly flagged as experimental), attached is 
a version without the micro_version check.

> [...]

Jérôme

[-- Attachment #2: 0001-avcodec-ffv1dec-reject-unsupported-ffv1-versions.patch --]
[-- Type: text/plain, Size: 1613 bytes --]

From dc0382709e548ef2514198bc866028066134d33e Mon Sep 17 00:00:00 2001
From: Jerome Martinez <jerome@mediaarea.net>
Date: Mon, 3 Apr 2023 00:04:53 +0200
Subject: [PATCH] avcodec/ffv1dec: reject unsupported ffv1 versions

And add similar check in libavformat/mxfenc
---
 libavcodec/ffv1dec.c | 5 +++++
 libavformat/mxfenc.c | 4 ++++
 2 files changed, 9 insertions(+)

diff --git a/libavcodec/ffv1dec.c b/libavcodec/ffv1dec.c
index 180d24e695..a3f9302233 100644
--- a/libavcodec/ffv1dec.c
+++ b/libavcodec/ffv1dec.c
@@ -439,6 +439,11 @@ static int read_extra_header(FFV1Context *f)
         av_log(f->avctx, AV_LOG_ERROR, "Invalid version in global header\n");
         return AVERROR_INVALIDDATA;
     }
+    if (f->version > 4) {
+        av_log(f->avctx, AV_LOG_ERROR, "unsupported version %d\n",
+            f->version);
+        return AVERROR_PATCHWELCOME;
+    }
     if (f->version > 2) {
         c->bytestream_end -= 4;
         f->micro_version = get_symbol(c, state, 0);
diff --git a/libavformat/mxfenc.c b/libavformat/mxfenc.c
index 0a1ae5353c..df5bb55ad8 100644
--- a/libavformat/mxfenc.c
+++ b/libavformat/mxfenc.c
@@ -2493,6 +2493,10 @@ static int mxf_parse_ffv1_frame(AVFormatContext *s, AVStream *st, AVPacket *pkt)
             av_log(s, AV_LOG_ERROR, "Invalid version in ffv1 global header\n");
             return 0;
         }
+        if (v > 4) {
+            av_log(s, AV_LOG_ERROR, "unsupported ffv1 version %d\n", v);
+            return 0;
+        }
         sc->micro_version = get_ffv1_unsigned_symbol(&c, state);
     } else {
         uint8_t keystate = 128;
-- 
2.13.3.windows.1


[-- Attachment #3: Type: text/plain, Size: 251 bytes --]

_______________________________________________
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] avformat/mxfenc: SMPTE RDD 48:2018 Amd 1:2022 (FFV1 in MXF) support

[Michael Niedermayer]

[-- Attachment #1.1: Type: text/plain, Size: 2011 bytes --]

On Tue, Apr 04, 2023 at 04:57:03PM +0200, Jerome Martinez wrote:
> On 04/04/2023 16:43, Michael Niedermayer wrote:
> > On Mon, Apr 03, 2023 at 12:07:06AM +0200, Jerome Martinez wrote:
> > > On 02/04/2023 22:07, Michael Niedermayer wrote:
> > > 
> > > +        if (f->version == 4 && f->micro_version > 2) {
> > > +            av_log(f->avctx, AV_LOG_ERROR, "unsupported version 4 micro_version %d\n",
> > > +                f->micro_version);
> > > +            return AVERROR_PATCHWELCOME;
> > > +        }
> > >       }
> > >       f->ac = get_symbol(c, state, 0);
> > you do not know if the decoder will have any problem with these files
> 
> 
> But you don't don't if the decoder will have no problem, it seems safer to
> me to reject something we are not sure to support.

"each new micro-version after this first stable variant is compatible with the previous micro-version: decoders **SHOULD NOT** reject FFV1 bitstreams due to an unknown micro-version equal or above the micro-version considered as stable."


[...]
>  libavcodec/ffv1dec.c |    5 +++++
>  libavformat/mxfenc.c |    4 ++++
>  2 files changed, 9 insertions(+)
> 9b094eb0bd0888725a4a3fac925ef1fa733a48c3  0001-avcodec-ffv1dec-reject-unsupported-ffv1-versions.patch
> From dc0382709e548ef2514198bc866028066134d33e Mon Sep 17 00:00:00 2001
> From: Jerome Martinez <jerome@mediaarea.net>
> Date: Mon, 3 Apr 2023 00:04:53 +0200
> Subject: [PATCH] avcodec/ffv1dec: reject unsupported ffv1 versions
> 
> And add similar check in libavformat/mxfenc
> ---
>  libavcodec/ffv1dec.c | 5 +++++
>  libavformat/mxfenc.c | 4 ++++
>  2 files changed, 9 insertions(+)

the patch is mostly ok
iam a bit undecided if a decoder change and a muxer bugfix belong in the
same patch, so do as you prefer

thx

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Good people do not need laws to tell them to act responsibly, while bad
people will find a way around the laws. -- Plato

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
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] avformat/mxfenc: SMPTE RDD 48:2018 Amd 1:2022 (FFV1 in MXF) support

[Jerome Martinez]

On 04/04/2023 19:32, Michael Niedermayer wrote:
> On Tue, Apr 04, 2023 at 04:57:03PM +0200, Jerome Martinez wrote:
>> On 04/04/2023 16:43, Michael Niedermayer wrote:
>>> On Mon, Apr 03, 2023 at 12:07:06AM +0200, Jerome Martinez wrote:
>>>> On 02/04/2023 22:07, Michael Niedermayer wrote:
>>>>
>>>> +        if (f->version == 4 && f->micro_version > 2) {
>>>> +            av_log(f->avctx, AV_LOG_ERROR, "unsupported version 4 micro_version %d\n",
>>>> +                f->micro_version);
>>>> +            return AVERROR_PATCHWELCOME;
>>>> +        }
>>>>        }
>>>>        f->ac = get_symbol(c, state, 0);
>>> you do not know if the decoder will have any problem with these files
>>
>> But you don't don't if the decoder will have no problem, it seems safer to
>> me to reject something we are not sure to support.
> "each new micro-version after this first stable variant is compatible with the previous micro-version: decoders **SHOULD NOT** reject FFV1 bitstreams due to an unknown micro-version equal or above the micro-version considered as stable."

This sentence is about a stable version, and version 4 is not stable, so 
FFV1 version 4 micro_version 3 is not necessarily compatible with FFV1 
version 4 micro_version 2, and so on, until we freeze the micro-version 
considered as stable. It is actually same for FFV1 version 3 e.g. "if 
(f->version == 3 && f->micro_version > 1 || f->version > 3) 
get_rac(&fs->c, (uint8_t[]) { 129 });" seems to say that version 3 
micro_version 2 is not compatible with version 3 micro_version 1 (and 
acceptable, only version 3 micro_version 5 and above must be compatible 
with version 3 micro_version 4 because micro_version 4 is the one 
flagged as stable).
Reason I suggested this micro_version check for version 4 (I didn't add 
it for version 3 because version 3 has a micro-version considered as 
stable).
Anyway, it is not important to me, just apply the patch you prefer.


>
>
> [...]
>>   libavcodec/ffv1dec.c |    5 +++++
>>   libavformat/mxfenc.c |    4 ++++
>>   2 files changed, 9 insertions(+)
>> 9b094eb0bd0888725a4a3fac925ef1fa733a48c3  0001-avcodec-ffv1dec-reject-unsupported-ffv1-versions.patch
>>  From dc0382709e548ef2514198bc866028066134d33e Mon Sep 17 00:00:00 2001
>> From: Jerome Martinez <jerome@mediaarea.net>
>> Date: Mon, 3 Apr 2023 00:04:53 +0200
>> Subject: [PATCH] avcodec/ffv1dec: reject unsupported ffv1 versions
>>
>> And add similar check in libavformat/mxfenc
>> ---
>>   libavcodec/ffv1dec.c | 5 +++++
>>   libavformat/mxfenc.c | 4 ++++
>>   2 files changed, 9 insertions(+)
> the patch is mostly ok
> iam a bit undecided if a decoder change and a muxer bugfix belong in the
> same patch, so do as you prefer

I think that both parts are about the same thing (and in the future both 
should be changed at the same time) but it is really subjective, let me 
know if you prefer that I send 2 separated patches and I'll do it, else 
please apply one of the already proposed patch.

Jérôme

_______________________________________________
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] avformat/mxfenc: SMPTE RDD 48:2018 Amd 1:2022 (FFV1 in MXF) support

[Michael Niedermayer]

[-- Attachment #1.1: Type: text/plain, Size: 5425 bytes --]

On Tue, Apr 04, 2023 at 11:37:54PM +0200, Jerome Martinez wrote:
> On 04/04/2023 19:32, Michael Niedermayer wrote:
> > On Tue, Apr 04, 2023 at 04:57:03PM +0200, Jerome Martinez wrote:
> > > On 04/04/2023 16:43, Michael Niedermayer wrote:
> > > > On Mon, Apr 03, 2023 at 12:07:06AM +0200, Jerome Martinez wrote:
> > > > > On 02/04/2023 22:07, Michael Niedermayer wrote:
> > > > > 
> > > > > +        if (f->version == 4 && f->micro_version > 2) {
> > > > > +            av_log(f->avctx, AV_LOG_ERROR, "unsupported version 4 micro_version %d\n",
> > > > > +                f->micro_version);
> > > > > +            return AVERROR_PATCHWELCOME;
> > > > > +        }
> > > > >        }
> > > > >        f->ac = get_symbol(c, state, 0);
> > > > you do not know if the decoder will have any problem with these files
> > > 
> > > But you don't don't if the decoder will have no problem, it seems safer to
> > > me to reject something we are not sure to support.
> > "each new micro-version after this first stable variant is compatible with the previous micro-version: decoders **SHOULD NOT** reject FFV1 bitstreams due to an unknown micro-version equal or above the micro-version considered as stable."
> 
> This sentence is about a stable version, and version 4 is not stable, so
> FFV1 version 4 micro_version 3 is not necessarily compatible with FFV1
> version 4 micro_version 2, and so on, until we freeze the micro-version
> considered as stable. It is actually same for FFV1 version 3 e.g. "if
> (f->version == 3 && f->micro_version > 1 || f->version > 3) get_rac(&fs->c,
> (uint8_t[]) { 129 });" seems to say that version 3 micro_version 2 is not
> compatible with version 3 micro_version 1 (and acceptable, only version 3
> micro_version 5 and above must be compatible with version 3 micro_version 4
> because micro_version 4 is the one flagged as stable).
> Reason I suggested this micro_version check for version 4 (I didn't add it
> for version 3 because version 3 has a micro-version considered as stable).
> Anyway, it is not important to me, just apply the patch you prefer.

Theres a practical problem with rejecting increased micro versions.
That problem is that we would not be able to introduce compatible changes
as even if they are compatible the micro_version itself would break all
decoders


> 
> 
> > 
> > 
> > [...]
> > >   libavcodec/ffv1dec.c |    5 +++++
> > >   libavformat/mxfenc.c |    4 ++++
> > >   2 files changed, 9 insertions(+)
> > > 9b094eb0bd0888725a4a3fac925ef1fa733a48c3  0001-avcodec-ffv1dec-reject-unsupported-ffv1-versions.patch
> > >  From dc0382709e548ef2514198bc866028066134d33e Mon Sep 17 00:00:00 2001
> > > From: Jerome Martinez <jerome@mediaarea.net>
> > > Date: Mon, 3 Apr 2023 00:04:53 +0200
> > > Subject: [PATCH] avcodec/ffv1dec: reject unsupported ffv1 versions
> > > 
> > > And add similar check in libavformat/mxfenc
> > > ---
> > >   libavcodec/ffv1dec.c | 5 +++++
> > >   libavformat/mxfenc.c | 4 ++++
> > >   2 files changed, 9 insertions(+)
> > the patch is mostly ok
> > iam a bit undecided if a decoder change and a muxer bugfix belong in the
> > same patch, so do as you prefer
> 
> I think that both parts are about the same thing (and in the future both
> should be changed at the same time) but it is really subjective, let me know
> if you prefer that I send 2 separated patches and I'll do it, else please
> apply one of the already proposed patch.

The problem is "same" "time"
In the world of libavcodec, "time" is specified in libavcodec/version.h
In the world of libavformat, "time" is specified in libavformat/version.h

Now if neither version is increased then you certainly can not assume
anything and any mixture can occurr. You can have before and after
libs mixed in any way on a system. For many changes this is ok
but sometimes the libavformat change needs the libavcodec feature.
So doing it in the same commit can be done here but it cannot be the
default (future) way to do it

If you bump the minor versions of both libs with a change then you can have
libavcodec  libavformat
before      before
after       before
after       after

If you bump the major versions of both libs with a change then you can have
before      before
after       after

the last variant is hard as major isnt bumped often.

so the way changes are introduced to 2 libs is to do it
first to libavcodec (this tests the after before case)
then subsequently to libavformat (this tests the after after case)
And with such seperate changes you just tested all legal combinations
without any need to think about it :)

OTOH if a commit does both libs together it requires an actual review
of what happens if not both libs are upgraded together on the user side.

The idea is simple, split it in 2 and things get naturally tested
and no need to spend any time on odd lib mixes as the only case is naturally
tested between the 2 commits.

Ill apply one of the patches as its ok here, but it might be "not ok" for
future changes to ffv1 in both libs, it depends on the exact changes

Thanks

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Concerning the gods, I have no means of knowing whether they exist or not
or of what sort they may be, because of the obscurity of the subject, and
the brevity of human life -- Protagoras

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
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] avformat/mxfenc: SMPTE RDD 48:2018 Amd 1:2022 (FFV1 in MXF) support

[Michael Niedermayer]

[-- Attachment #1.1: Type: text/plain, Size: 5722 bytes --]

On Wed, Apr 05, 2023 at 02:31:16PM +0200, Michael Niedermayer wrote:
> On Tue, Apr 04, 2023 at 11:37:54PM +0200, Jerome Martinez wrote:
> > On 04/04/2023 19:32, Michael Niedermayer wrote:
> > > On Tue, Apr 04, 2023 at 04:57:03PM +0200, Jerome Martinez wrote:
> > > > On 04/04/2023 16:43, Michael Niedermayer wrote:
> > > > > On Mon, Apr 03, 2023 at 12:07:06AM +0200, Jerome Martinez wrote:
> > > > > > On 02/04/2023 22:07, Michael Niedermayer wrote:
> > > > > > 
> > > > > > +        if (f->version == 4 && f->micro_version > 2) {
> > > > > > +            av_log(f->avctx, AV_LOG_ERROR, "unsupported version 4 micro_version %d\n",
> > > > > > +                f->micro_version);
> > > > > > +            return AVERROR_PATCHWELCOME;
> > > > > > +        }
> > > > > >        }
> > > > > >        f->ac = get_symbol(c, state, 0);
> > > > > you do not know if the decoder will have any problem with these files
> > > > 
> > > > But you don't don't if the decoder will have no problem, it seems safer to
> > > > me to reject something we are not sure to support.
> > > "each new micro-version after this first stable variant is compatible with the previous micro-version: decoders **SHOULD NOT** reject FFV1 bitstreams due to an unknown micro-version equal or above the micro-version considered as stable."
> > 
> > This sentence is about a stable version, and version 4 is not stable, so
> > FFV1 version 4 micro_version 3 is not necessarily compatible with FFV1
> > version 4 micro_version 2, and so on, until we freeze the micro-version
> > considered as stable. It is actually same for FFV1 version 3 e.g. "if
> > (f->version == 3 && f->micro_version > 1 || f->version > 3) get_rac(&fs->c,
> > (uint8_t[]) { 129 });" seems to say that version 3 micro_version 2 is not
> > compatible with version 3 micro_version 1 (and acceptable, only version 3
> > micro_version 5 and above must be compatible with version 3 micro_version 4
> > because micro_version 4 is the one flagged as stable).
> > Reason I suggested this micro_version check for version 4 (I didn't add it
> > for version 3 because version 3 has a micro-version considered as stable).
> > Anyway, it is not important to me, just apply the patch you prefer.
> 
> Theres a practical problem with rejecting increased micro versions.
> That problem is that we would not be able to introduce compatible changes
> as even if they are compatible the micro_version itself would break all
> decoders
> 
> 
> > 
> > 
> > > 
> > > 
> > > [...]
> > > >   libavcodec/ffv1dec.c |    5 +++++
> > > >   libavformat/mxfenc.c |    4 ++++
> > > >   2 files changed, 9 insertions(+)
> > > > 9b094eb0bd0888725a4a3fac925ef1fa733a48c3  0001-avcodec-ffv1dec-reject-unsupported-ffv1-versions.patch
> > > >  From dc0382709e548ef2514198bc866028066134d33e Mon Sep 17 00:00:00 2001
> > > > From: Jerome Martinez <jerome@mediaarea.net>
> > > > Date: Mon, 3 Apr 2023 00:04:53 +0200
> > > > Subject: [PATCH] avcodec/ffv1dec: reject unsupported ffv1 versions
> > > > 
> > > > And add similar check in libavformat/mxfenc
> > > > ---
> > > >   libavcodec/ffv1dec.c | 5 +++++
> > > >   libavformat/mxfenc.c | 4 ++++
> > > >   2 files changed, 9 insertions(+)
> > > the patch is mostly ok
> > > iam a bit undecided if a decoder change and a muxer bugfix belong in the
> > > same patch, so do as you prefer
> > 
> > I think that both parts are about the same thing (and in the future both
> > should be changed at the same time) but it is really subjective, let me know
> > if you prefer that I send 2 separated patches and I'll do it, else please
> > apply one of the already proposed patch.
> 
> The problem is "same" "time"
> In the world of libavcodec, "time" is specified in libavcodec/version.h
> In the world of libavformat, "time" is specified in libavformat/version.h
> 
> Now if neither version is increased then you certainly can not assume
> anything and any mixture can occurr. You can have before and after
> libs mixed in any way on a system. For many changes this is ok
> but sometimes the libavformat change needs the libavcodec feature.
> So doing it in the same commit can be done here but it cannot be the
> default (future) way to do it
> 
> If you bump the minor versions of both libs with a change then you can have
> libavcodec  libavformat
> before      before
> after       before
> after       after
> 
> If you bump the major versions of both libs with a change then you can have
> before      before
> after       after
> 
> the last variant is hard as major isnt bumped often.
> 
> so the way changes are introduced to 2 libs is to do it
> first to libavcodec (this tests the after before case)
> then subsequently to libavformat (this tests the after after case)
> And with such seperate changes you just tested all legal combinations
> without any need to think about it :)
> 
> OTOH if a commit does both libs together it requires an actual review
> of what happens if not both libs are upgraded together on the user side.
> 
> The idea is simple, split it in 2 and things get naturally tested
> and no need to spend any time on odd lib mixes as the only case is naturally
> tested between the 2 commits.
> 

> Ill apply one of the patches as its ok here, but it might be "not ok" for
> future changes to ffv1 in both libs, it depends on the exact changes

Changed my mind, ill push it splited in 2. Its more consistent with how
such changes have been done, even when its not needed here

thx

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

He who knows, does not speak. He who speaks, does not know. -- Lao Tsu

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

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