| Message ID | b7f281fb-2cd9-a85c-1868-0c824b150634@mediaarea.net |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel] avformat/mxfenc: SMPTE RDD 48:2018 Amd 1:2022 (FFV1 in MXF) support | expand |
| Context | Check | Description |
|---|---|---|
| yinshiyou/configure_loongarch64 | warning | Failed to apply patch |
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
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
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
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
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
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).
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
> 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
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
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 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
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
> 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
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
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 [...]
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.
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 [...]
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. 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; } }
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 [...]
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 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;
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 [...]
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 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;
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 [...]
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
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 [...]
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 [...]
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"
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 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