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 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 > 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