| Message ID | 0af7e930-9ec7-dc81-95bc-939eb21e2da0@mediaarea.net |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel] avformat/mxfenc: fix stored/sampled/displayed width/height | expand |
| Context | Check | Description |
|---|---|---|
| yinshiyou/configure_loongarch64 | warning | Failed to apply patch |
On Sat, Jan 14, 2023 at 04:48:10PM +0100, Jerome Martinez wrote: > Before the patch: > - stored values were rounded to upper 16 multiple also for formats not using > macroblocks (should be st->codecpar->width and st->codecpar->height when not > MPEG formats; note that I found no other muxer doing the rounding for AVC, > only for MPEG-2 Video, but I find no reason in specs for doing the > difference so I kept the rounding for AVC) > - sampled and displayed widths were stored width (should be > st->codecpar->width like it is already done for height, with the DV50/100 > exception) > > Could be tested with e.g. > - fixed stored width (1912 instead of 1920) and height (1080 instead of > 1088) not multiple of 16 : > ffmpeg -f lavfi -i testsrc=duration=1:size=1912x1080 -c:v jpeg2000 > test_prores.mxf > - fixed sampled/displayed width (1912 instead of 1920): > ffmpeg -f lavfi -i testsrc=duration=1:size=1912x1080 -c:v mpeg2video > test_mpeg2video.mxf > mxfenc.c | 28 ++++++++++++++++++++++------ > 1 file changed, 22 insertions(+), 6 deletions(-) > ff6497b7983ccb5ec7555b55ad0040a2c065d6d3 0001-avformat-mxfenc-fix-stored-sampled-displayed-width-h.patch > From cda353059886182aab2e258023c4d027c448344b Mon Sep 17 00:00:00 2001 > From: Jerome Martinez <jerome@mediaarea.net> > Date: Sat, 14 Jan 2023 13:32:36 +0100 > Subject: [PATCH] avformat/mxfenc: fix stored/sampled/displayed width/height > > Stored values are rounded to upper 16 multiple only for MPEG related formats > Sampled and displayed widths are codecpar ones (with DV exception) > --- > libavformat/mxfenc.c | 28 ++++++++++++++++++++++------ > 1 file changed, 22 insertions(+), 6 deletions(-) > > diff --git a/libavformat/mxfenc.c b/libavformat/mxfenc.c > index 58c551c83c..0b7e83ba4d 100644 > --- a/libavformat/mxfenc.c > +++ b/libavformat/mxfenc.c > @@ -1109,8 +1109,9 @@ static int64_t mxf_write_cdci_common(AVFormatContext *s, AVStream *st, const UID > { > MXFStreamContext *sc = st->priv_data; > AVIOContext *pb = s->pb; > - int stored_width = 0; > - int stored_height = (st->codecpar->height+15)/16*16; > + int stored_width = st->codecpar->width; > + int stored_height = st->codecpar->height; > + int display_width; > int display_height; > int f1, f2; > const MXFCodecUL *color_primaries_ul; > @@ -1129,12 +1130,25 @@ static int64_t mxf_write_cdci_common(AVFormatContext *s, AVStream *st, const UID > else if (st->codecpar->height == 720) > stored_width = 1280; > } > - if (!stored_width) > - stored_width = (st->codecpar->width+15)/16*16; > + display_width = stored_width; > > + switch (st->codecpar->codec_id) { > + case AV_CODEC_ID_MPEG2VIDEO: > + case AV_CODEC_ID_DVVIDEO: > + case AV_CODEC_ID_H264: > + //Based on 16x16 macroblocks > + stored_width = (stored_width+15)/16*16; > + stored_height = (stored_height+15)/16*16; If this is supposed to match the actual macroblocks, then this would have to consider field pictures and interlacing as it differs from progressive thx [...]
On 14/01/2023 21:04, Michael Niedermayer wrote: > On Sat, Jan 14, 2023 at 04:48:10PM +0100, Jerome Martinez wrote: > [...] >> + stored_height = (stored_height+15)/16*16; > If this is supposed to match the actual macroblocks, then this would > have to consider field pictures and interlacing as it differs from > progressive There is no change on that side, no addition, no change about it, this code is moved, and interlace is already managed in the current code: //Stored height mxf_write_local_tag(s, 4, 0x3202); avio_wb32(pb, stored_height>>sc->interlaced); [...] //Display height mxf_write_local_tag(s, 4, 0x3208); avio_wb32(pb, display_height>>sc->interlaced); The patch disables the rounding for some formats, it is not intended to change the behavior for MPEG-2 Video and AVC formats as it seems fine (FFmpeg has the same behavior as other muxers, at least for MPEG-2 Video, also for interlaced content).
On Sun, Jan 15, 2023 at 03:24:37PM +0100, Jerome Martinez wrote: > On 14/01/2023 21:04, Michael Niedermayer wrote: > > On Sat, Jan 14, 2023 at 04:48:10PM +0100, Jerome Martinez wrote: > > [...] > > > + stored_height = (stored_height+15)/16*16; > > If this is supposed to match the actual macroblocks, then this would > > have to consider field pictures and interlacing as it differs from > > progressive > > There is no change on that side, no addition, no change about it, this code > is moved, and interlace is already managed in the current code: > > //Stored height > mxf_write_local_tag(s, 4, 0x3202); > avio_wb32(pb, stored_height>>sc->interlaced); > [...] > //Display height > mxf_write_local_tag(s, 4, 0x3208); > avio_wb32(pb, display_height>>sc->interlaced); > > The patch disables the rounding for some formats, it is not intended to > change the behavior for MPEG-2 Video and AVC formats as it seems fine > (FFmpeg has the same behavior as other muxers, at least for MPEG-2 Video, > also for interlaced content). ok thx [...]
lör 2023-01-14 klockan 16:48 +0100 skrev Jerome Martinez: > Before the patch: > - stored values were rounded to upper 16 multiple also for formats > not > using macroblocks (should be st->codecpar->width and > st->codecpar->height when not MPEG formats; note that I found no > other > muxer doing the rounding for AVC, only for MPEG-2 Video, but I find > no > reason in specs for doing the difference so I kept the rounding for > AVC) > - sampled and displayed widths were stored width (should be > st->codecpar->width like it is already done for height, with the > DV50/100 exception) Another option might be to omit these values for non-macroblock codecs. There is also potentially one use-value: when remuxing BMP to MXF it may be necessary to deal with BMP's 16-bit alignment. I'm not sure if this happens in the wild, and we certainly don't support muxing it. Another very close reading of the spec seems appropriate. /Tomas
>Before the patch: >- stored values were rounded to upper 16 multiple also for formats not using macroblocks (should be st->codecpar->width and >st->codecpar->height when not MPEG formats; note that I found no other >muxer doing the rounding for AVC, only for MPEG-2 Video, but I find no reason in specs for doing the difference so I kept the rounding for AVC) >- sampled and displayed widths were stored width (should be >st->codecpar->width like it is already done for height, with the >DV50/100 exception) The width is one thing; for whatever reason, there is a divergence between DV100 on one hand and AVCI/XDCAMHD35 on the other. In my understanding, in current practices, DV obey s337 (stored width includes scaling) but xdcam&avci does not, so current code is fine but maybe this is an opportunity to document this ? The height is another topic, and in my information (checked against some samples from K2/Harmonic/bmx), DV height should not be rounded : maybe this patch is an opportunity to fix this ? Note: please mind update fate (make fate-lavf-mxf_opatom GEN=1). Nicolas
On 16/01/2023 14:50, Tomas Härdin wrote: > lör 2023-01-14 klockan 16:48 +0100 skrev Jerome Martinez: >> Before the patch: >> - stored values were rounded to upper 16 multiple also for formats >> not >> using macroblocks (should be st->codecpar->width and >> st->codecpar->height when not MPEG formats; note that I found no >> other >> muxer doing the rounding for AVC, only for MPEG-2 Video, but I find >> no >> reason in specs for doing the difference so I kept the rounding for >> AVC) >> - sampled and displayed widths were stored width (should be >> st->codecpar->width like it is already done for height, with the >> DV50/100 exception) > Another option might be to omit these values for non-macroblock codecs. I planned to send a patch for removing all redundant information (sampled width/height is stored width/height by default etc), but as it is changing the current behavior of FFmpeg so can be more controversial I preferred to have it in a different patch. After this patch it would be only a bunch of "if (sampled!=stored)" additions. Would it be fine to do that separately? > There is also potentially one use-value: when remuxing BMP to MXF it > may be necessary to deal with BMP's 16-bit alignment. I'm not sure if > this happens in the wild, and we certainly don't support muxing it. I don't get what I should change in the patch for that, AFAIK I don't touch stuff around that. > Another very close reading of the spec seems appropriate. Tried to do so, as well as checking the other files I have from other muxers. When I don't get the difference between what is in specs and FFmpeg or other muxer behavior, I let current FFmpeg behavior untouched. Jérôme
On 16/01/2023 15:00, Nicolas Gaullier wrote: >> Before the patch: >> - stored values were rounded to upper 16 multiple also for formats not using macroblocks (should be st->codecpar->width and >> st->codecpar->height when not MPEG formats; note that I found no other >> muxer doing the rounding for AVC, only for MPEG-2 Video, but I find no reason in specs for doing the difference so I kept the rounding for AVC) >> - sampled and displayed widths were stored width (should be >> st->codecpar->width like it is already done for height, with the >> DV50/100 exception) > The width is one thing; for whatever reason, there is a divergence between DV100 on one hand and AVCI/XDCAMHD35 on the other. In my understanding, in current practices, DV obey s337 (stored width includes scaling) but xdcam&avci does not, so current code is fine but maybe this is an opportunity to document this ? AFAIK: - DV in MXF: found old Omneon with no scaling for stored value, no sampled value (so stored value), scaled value for displayed value, old Quantel with scaling everywhere. From my understanding of spec, I would keep the scaling. - MPEG-2 Video including XDCAMHD35 in MXF obey "including any decoder scaling or padding" wording with a 16x16 rounding for height, I have no file not 1920 or 3840 width - AVC in MXF: found old Omneon or old Quantel or old Telestream with no padding value for stored value (height of 540 for interlaced). I don't understand why it is not same as with MPEG-2 Video so I don't touch FFmpeg behavior there (rounding). Actually checking again SMPTE ST 381-2013, there is an explicit example: "1088: 1080-line progressive". Do you want me to add a comment line e.g. "obey 'including any decoder scaling or padding' from SMPTE ST 377"? > The height is another topic, and in my information (checked against some samples from K2/Harmonic/bmx), DV height should not be rounded : maybe this patch is an opportunity to fix this ? I don't have DV in MXF with non multiple of 16 (I thought that DV is only 720x576 or 720x480 or 1280x720 or 1920x1080, all values multiple of 16) and don't know about video encoding in DV so I didn't want to change the behavior of FFmpeg when I don't know, but case AV_CODEC_ID_DVVIDEO: line could be definitely removed if it is fine for you. > > Note: please mind update fate (make fate-lavf-mxf_opatom GEN=1). Checked (only the stored height changes, from 1088 to 1080 for this DNxHD file) and fate updated for next patch.
>> The width is one thing; for whatever reason, there is a divergence between DV100 on one hand and AVCI/XDCAMHD35 on the other. In my understanding, in current practices, DV obey s337 (stored width includes scaling) but >xdcam&avci does not, so current code is fine but maybe this is an opportunity to document this ? > >AFAIK: >- DV in MXF: found old Omneon with no scaling for stored value, no sampled value (so stored value), scaled value for displayed value, old Quantel with scaling everywhere. From my understanding of spec, I would keep the scaling. >- MPEG-2 Video including XDCAMHD35 in MXF obey "including any decoder scaling or padding" wording with a 16x16 rounding for height, I have no file not 1920 or 3840 width >- AVC in MXF: found old Omneon or old Quantel or old Telestream with no padding value for stored value (height of 540 for interlaced). I don't understand why it is not same as with MPEG-2 Video so I don't touch FFmpeg behavior >there (rounding). Actually checking again SMPTE ST 381-2013, there is an explicit example: "1088: 1080-line progressive". I totally agree they are so many weird things in the wild, particularly looking at some early implementations. I also have fully broken DV100 and XDCAMHD35 Omneon records with release v6.1 (2010) at the early stages of HD, but it was fixed afterwards (with many other issues too!). Looking at GVG, 1440x1088i stored size was implemented from the early beginnings (2010 too) : sample clips are still available here : http://www.gvgdevelopers.com/concrete/products/k2/test_clips/ There is also "kind of" reference sony implementation available here both for xdcamhd35/avc-1440: https://www.sonycreativesoftware.com/catalystbrowse Anyway, I think we all agree not to change anything related to MPEG2 and AVC. >I don't have DV in MXF with non multiple of 16 (I thought that DV is only 720x576 or 720x480 or 1280x720 or 1920x1080, all values multiple of >16) and don't know about video encoding in DV so I didn't want to change the behavior of FFmpeg when I don't know, but case AV_CODEC_ID_DVVIDEO: >line could be definitely removed if it is fine for you. DV is questionable. Currently, the dust is under the rug (as a defaults behaviour), which is an issue with very little concern. Now, with the patch, the dust become visible, the DV rule is made explicit and moreover it is presented as an exception, sharing code with macroblock codecs... I think it is time to fix, even if it was not your prior intention. I don't have an extensive experience with DV too, I just have samples here and there like you, but it seems we share the same information. Let see if someone else react and ask for keeping the current 1088 lines for DV stored height, but if nobody does, I think it should be okay. > Do you want me to add a comment line e.g. "obey 'including any decoder scaling or padding' from SMPTE ST 377"? I am not a core developer and will let others give their feedback. My personal opinion is that the spec is supposed to be well known and does not require commenting, but that it would be interesting to explicit why we make a difference between DV and MPEG2/AVC. And personally, I don't have the answer to this question. If nobody has one, maybe a comment could say "because this is the observed common practice". Nicolas
mån 2023-01-16 klockan 15:28 +0100 skrev Jerome Martinez: > On 16/01/2023 14:50, Tomas Härdin wrote: > > lör 2023-01-14 klockan 16:48 +0100 skrev Jerome Martinez: > > > Before the patch: > > > - stored values were rounded to upper 16 multiple also for > > > formats > > > not > > > using macroblocks (should be st->codecpar->width and > > > st->codecpar->height when not MPEG formats; note that I found no > > > other > > > muxer doing the rounding for AVC, only for MPEG-2 Video, but I > > > find > > > no > > > reason in specs for doing the difference so I kept the rounding > > > for > > > AVC) > > > - sampled and displayed widths were stored width (should be > > > st->codecpar->width like it is already done for height, with the > > > DV50/100 exception) > > Another option might be to omit these values for non-macroblock > > codecs. > > I planned to send a patch for removing all redundant information > (sampled width/height is stored width/height by default etc), but as > it > is changing the current behavior of FFmpeg so can be more > controversial > I preferred to have it in a different patch. Such information may or may not be redundant depending on context. Which is part of why MXF is such a headache. Outputting as correct information as possible is probably the best approach. > After this patch it would be only a bunch of "if (sampled!=stored)" > additions. > Would it be fine to do that separately? I don't know. The problem here is that we don't know what workflows users have. On the other hand if we're bold and remove redundant information then we'll learn what users are actually relying on it. > > There is also potentially one use-value: when remuxing BMP to MXF > > it > > may be necessary to deal with BMP's 16-bit alignment. I'm not sure > > if > > this happens in the wild, and we certainly don't support muxing it. > > I don't get what I should change in the patch for that, AFAIK I don't > touch stuff around that. No need to do anything, it's just me noting when it might be the case that stored != sampled for non-MB formats. > > Another very close reading of the spec seems appropriate. > > Tried to do so, as well as checking the other files I have from other > muxers. > When I don't get the difference between what is in specs and FFmpeg > or > other muxer behavior, I let current FFmpeg behavior untouched. Ugh, cargo culting. Not that I'm not guilty of the same.. /Tomas
>>> The width is one thing; for whatever reason, there is a divergence between DV100 on one hand and AVCI/XDCAMHD35 on the other. In my understanding, in current practices, DV obey s337 (stored width includes scaling) but >xdcam&avci does not, so current code is fine >but maybe this is an opportunity to document this ? >> >>AFAIK: >>- DV in MXF: found old Omneon with no scaling for stored value, no sampled value (so stored value), scaled value for displayed value, old Quantel with scaling everywhere. From my understanding of spec, I would keep the scaling. >>- MPEG-2 Video including XDCAMHD35 in MXF obey "including any decoder >>scaling or padding" wording with a 16x16 rounding for height, I have no >>file not 1920 or 3840 width >>- AVC in MXF: found old Omneon or old Quantel or old Telestream with no padding value for stored value (height of 540 for interlaced). I don't understand why it is not same as with MPEG-2 Video so I don't touch FFmpeg behavior >there (rounding). Actually checking >again SMPTE ST 381-2013, there is an explicit example: "1088: 1080-line progressive". > >I totally agree they are so many weird things in the wild, particularly looking at some early implementations. I also have fully broken DV100 and XDCAMHD35 Omneon records with release v6.1 (2010) at the early stages of HD, but it was fixed afterwards (with many other >issues too!). Looking at GVG, 1440x1088i stored size was implemented from the early beginnings (2010 too) : sample clips are still available here : http://www.gvgdevelopers.com/concrete/products/k2/test_clips/ >There is also "kind of" reference sony implementation available here both for xdcamhd35/avc-1440: https://www.sonycreativesoftware.com/catalystbrowse >Anyway, I think we all agree not to change anything related to MPEG2 and AVC. > >>I don't have DV in MXF with non multiple of 16 (I thought that DV is >>only 720x576 or 720x480 or 1280x720 or 1920x1080, all values multiple of >>16) and don't know about video encoding in DV so I didn't want to change the behavior of FFmpeg when I don't know, but case AV_CODEC_ID_DVVIDEO: >>line could be definitely removed if it is fine for you. >DV is questionable. Currently, the dust is under the rug (as a defaults behaviour), which is an issue with very little concern. >Now, with the patch, the dust become visible, the DV rule is made explicit and moreover it is presented as an exception, sharing code with macroblock codecs... I think it is time to fix, even if it was not your prior intention. >I don't have an extensive experience with DV too, I just have samples here and there like you, but it seems we share the same information. >Let see if someone else react and ask for keeping the current 1088 lines for DV stored height, but if nobody does, I think it should be okay. > >> Do you want me to add a comment line e.g. "obey 'including any decoder scaling or padding' from SMPTE ST 377"? >I am not a core developer and will let others give their feedback. My personal opinion is that the spec is supposed to be well known and does not require commenting, but that it would be interesting to explicit why we make a difference between DV and MPEG2/AVC. And >personally, I don't have the answer to this question. If nobody has one, maybe a comment could say "because this is the observed common practice". > >Nicolas Some weeks later now and no replies, maybe time to go on ? I think the "case AV_CODEC_ID_DVVIDEO:" can be removed as discussed, fate updated and that should be ok for everybody. (Ideally, it could have been an opportunity to document why we have this "DV exception", but I understand it is not very comfortable to write as there is no meaningful reason, so forget about this, this won't hold up the patch anyway) For information, there was a long thread recently on ffmpeg-user about a "bug" in dnxhd stored_height (will be fixed with your patch): https://ffmpeg.org/pipermail/ffmpeg-user/2023-February/056111.html Nicolas
On Mon, 6 Mar 2023, Nicolas Gaullier wrote: >>>> The width is one thing; for whatever reason, there is a divergence between DV100 on one hand and AVCI/XDCAMHD35 on the other. In my understanding, in current practices, DV obey s337 (stored width includes scaling) but >xdcam&avci does not, so current code is fine >but maybe this is an opportunity to document this ? >>> >>> AFAIK: >>> - DV in MXF: found old Omneon with no scaling for stored value, no sampled value (so stored value), scaled value for displayed value, old Quantel with scaling everywhere. From my understanding of spec, I would keep the scaling. >>> - MPEG-2 Video including XDCAMHD35 in MXF obey "including any decoder >>> scaling or padding" wording with a 16x16 rounding for height, I have no >>> file not 1920 or 3840 width >>> - AVC in MXF: found old Omneon or old Quantel or old Telestream with no padding value for stored value (height of 540 for interlaced). I don't understand why it is not same as with MPEG-2 Video so I don't touch FFmpeg behavior >there (rounding). Actually checking >again SMPTE ST 381-2013, there is an explicit example: "1088: 1080-line progressive". >> >> I totally agree they are so many weird things in the wild, particularly looking at some early implementations. I also have fully broken DV100 and XDCAMHD35 Omneon records with release v6.1 (2010) at the early stages of HD, but it was fixed afterwards (with many other >issues too!). Looking at GVG, 1440x1088i stored size was implemented from the early beginnings (2010 too) : sample clips are still available here : http://www.gvgdevelopers.com/concrete/products/k2/test_clips/ >> There is also "kind of" reference sony implementation available here both for xdcamhd35/avc-1440: https://www.sonycreativesoftware.com/catalystbrowse >> Anyway, I think we all agree not to change anything related to MPEG2 and AVC. >> >>> I don't have DV in MXF with non multiple of 16 (I thought that DV is >>> only 720x576 or 720x480 or 1280x720 or 1920x1080, all values multiple of >>> 16) and don't know about video encoding in DV so I didn't want to change the behavior of FFmpeg when I don't know, but case AV_CODEC_ID_DVVIDEO: >>> line could be definitely removed if it is fine for you. >> DV is questionable. Currently, the dust is under the rug (as a defaults behaviour), which is an issue with very little concern. >> Now, with the patch, the dust become visible, the DV rule is made explicit and moreover it is presented as an exception, sharing code with macroblock codecs... I think it is time to fix, even if it was not your prior intention. >> I don't have an extensive experience with DV too, I just have samples here and there like you, but it seems we share the same information. >> Let see if someone else react and ask for keeping the current 1088 lines for DV stored height, but if nobody does, I think it should be okay. >> >>> Do you want me to add a comment line e.g. "obey 'including any decoder scaling or padding' from SMPTE ST 377"? >> I am not a core developer and will let others give their feedback. My personal opinion is that the spec is supposed to be well known and does not require commenting, but that it would be interesting to explicit why we make a difference between DV and MPEG2/AVC. And >personally, I don't have the answer to this question. If nobody has one, maybe a comment could say "because this is the observed common practice". >> >> Nicolas > > Some weeks later now and no replies, maybe time to go on ? > I think the "case AV_CODEC_ID_DVVIDEO:" can be removed as discussed, fate updated and that should be ok for everybody. > (Ideally, it could have been an opportunity to document why we have this "DV exception", but I understand it is not very comfortable to write as there is no meaningful reason, so forget about this, this won't hold up the patch anyway) > For information, there was a long thread recently on ffmpeg-user about a "bug" in dnxhd stored_height (will be fixed with your patch): > https://ffmpeg.org/pipermail/ffmpeg-user/2023-February/056111.html Will apply the patch in a couple of days unless somebody objects. If you want to change DV height (seems reasonable), please send a follow up patch with fate updates after that. Thanks, Marton
On Fri, 10 Mar 2023, Marton Balint wrote: > > > On Mon, 6 Mar 2023, Nicolas Gaullier wrote: > >>>>> The width is one thing; for whatever reason, there is a divergence >>>>> between DV100 on one hand and AVCI/XDCAMHD35 on the other. In my >>>>> understanding, in current practices, DV obey s337 (stored width >>>>> includes scaling) but >xdcam&avci does not, so current code is fine >>>>> >but maybe this is an opportunity to document this ? >>>> >>>> AFAIK: >>>> - DV in MXF: found old Omneon with no scaling for stored value, no >>>> sampled value (so stored value), scaled value for displayed value, old >>>> Quantel with scaling everywhere. From my understanding of spec, I would >>>> keep the scaling. >>>> - MPEG-2 Video including XDCAMHD35 in MXF obey "including any decoder >>>> scaling or padding" wording with a 16x16 rounding for height, I have no >>>> file not 1920 or 3840 width >>>> - AVC in MXF: found old Omneon or old Quantel or old Telestream with no >>>> padding value for stored value (height of 540 for interlaced). I don't >>>> understand why it is not same as with MPEG-2 Video so I don't touch >>>> FFmpeg behavior >there (rounding). Actually checking >again SMPTE ST >>>> 381-2013, there is an explicit example: "1088: 1080-line progressive". >>> >>> I totally agree they are so many weird things in the wild, particularly >>> looking at some early implementations. I also have fully broken DV100 and >>> XDCAMHD35 Omneon records with release v6.1 (2010) at the early stages of >>> HD, but it was fixed afterwards (with many other >issues too!). Looking >>> at GVG, 1440x1088i stored size was implemented from the early beginnings >>> (2010 too) : sample clips are still available here : >>> http://www.gvgdevelopers.com/concrete/products/k2/test_clips/ >>> There is also "kind of" reference sony implementation available here both >>> for xdcamhd35/avc-1440: >>> https://www.sonycreativesoftware.com/catalystbrowse >>> Anyway, I think we all agree not to change anything related to MPEG2 and >>> AVC. >>> >>>> I don't have DV in MXF with non multiple of 16 (I thought that DV is >>>> only 720x576 or 720x480 or 1280x720 or 1920x1080, all values multiple of >>>> 16) and don't know about video encoding in DV so I didn't want to change >>>> the behavior of FFmpeg when I don't know, but case AV_CODEC_ID_DVVIDEO: >>>> line could be definitely removed if it is fine for you. >>> DV is questionable. Currently, the dust is under the rug (as a defaults >>> behaviour), which is an issue with very little concern. >>> Now, with the patch, the dust become visible, the DV rule is made >>> explicit and moreover it is presented as an exception, sharing code with >>> macroblock codecs... I think it is time to fix, even if it was not your >>> prior intention. >>> I don't have an extensive experience with DV too, I just have samples >>> here and there like you, but it seems we share the same information. >>> Let see if someone else react and ask for keeping the current 1088 lines >>> for DV stored height, but if nobody does, I think it should be okay. >>> >>>> Do you want me to add a comment line e.g. "obey 'including any decoder >>>> scaling or padding' from SMPTE ST 377"? >>> I am not a core developer and will let others give their feedback. My >>> personal opinion is that the spec is supposed to be well known and does >>> not require commenting, but that it would be interesting to explicit why >>> we make a difference between DV and MPEG2/AVC. And >personally, I don't >>> have the answer to this question. If nobody has one, maybe a comment >>> could say "because this is the observed common practice". >>> >>> Nicolas >> >> Some weeks later now and no replies, maybe time to go on ? >> I think the "case AV_CODEC_ID_DVVIDEO:" can be removed as discussed, fate >> updated and that should be ok for everybody. >> (Ideally, it could have been an opportunity to document why we have this >> "DV exception", but I understand it is not very comfortable to write as >> there is no meaningful reason, so forget about this, this won't hold up >> the patch anyway) >> For information, there was a long thread recently on ffmpeg-user about a >> "bug" in dnxhd stored_height (will be fixed with your patch): >> https://ffmpeg.org/pipermail/ffmpeg-user/2023-February/056111.html > > Will apply the patch in a couple of days unless somebody objects. If you want > to change DV height (seems reasonable), please send a follow up patch with > fate updates after that. Oh, this patch needs a fate update as well. On that note, DNXHD is a macroblock-based codec, so why are we changing 1088 height to 1080? I could ask the same for ProRes. The patch should explain better why those need to be changed, if they do. Thanks, Marton
On Mon, Mar 13, 2023 at 3:30 PM Marton Balint <cus@passwd.hu> wrote: > > > > On Fri, 10 Mar 2023, Marton Balint wrote: > > > > > > > On Mon, 6 Mar 2023, Nicolas Gaullier wrote: > > > >>>>> The width is one thing; for whatever reason, there is a divergence > >>>>> between DV100 on one hand and AVCI/XDCAMHD35 on the other. In my > >>>>> understanding, in current practices, DV obey s337 (stored width > >>>>> includes scaling) but >xdcam&avci does not, so current code is fine > >>>>> >but maybe this is an opportunity to document this ? > >>>> > >>>> AFAIK: > >>>> - DV in MXF: found old Omneon with no scaling for stored value, no > >>>> sampled value (so stored value), scaled value for displayed value, old > >>>> Quantel with scaling everywhere. From my understanding of spec, I would > >>>> keep the scaling. > >>>> - MPEG-2 Video including XDCAMHD35 in MXF obey "including any decoder > >>>> scaling or padding" wording with a 16x16 rounding for height, I have no > >>>> file not 1920 or 3840 width > >>>> - AVC in MXF: found old Omneon or old Quantel or old Telestream with no > >>>> padding value for stored value (height of 540 for interlaced). I don't > >>>> understand why it is not same as with MPEG-2 Video so I don't touch > >>>> FFmpeg behavior >there (rounding). Actually checking >again SMPTE ST > >>>> 381-2013, there is an explicit example: "1088: 1080-line progressive". > >>> > >>> I totally agree they are so many weird things in the wild, particularly > >>> looking at some early implementations. I also have fully broken DV100 and > >>> XDCAMHD35 Omneon records with release v6.1 (2010) at the early stages of > >>> HD, but it was fixed afterwards (with many other >issues too!). Looking > >>> at GVG, 1440x1088i stored size was implemented from the early beginnings > >>> (2010 too) : sample clips are still available here : > >>> http://www.gvgdevelopers.com/concrete/products/k2/test_clips/ > >>> There is also "kind of" reference sony implementation available here both > >>> for xdcamhd35/avc-1440: > >>> https://www.sonycreativesoftware.com/catalystbrowse > >>> Anyway, I think we all agree not to change anything related to MPEG2 and > >>> AVC. > >>> > >>>> I don't have DV in MXF with non multiple of 16 (I thought that DV is > >>>> only 720x576 or 720x480 or 1280x720 or 1920x1080, all values multiple of > >>>> 16) and don't know about video encoding in DV so I didn't want to change > >>>> the behavior of FFmpeg when I don't know, but case AV_CODEC_ID_DVVIDEO: > >>>> line could be definitely removed if it is fine for you. > >>> DV is questionable. Currently, the dust is under the rug (as a defaults > >>> behaviour), which is an issue with very little concern. > >>> Now, with the patch, the dust become visible, the DV rule is made > >>> explicit and moreover it is presented as an exception, sharing code with > >>> macroblock codecs... I think it is time to fix, even if it was not your > >>> prior intention. > >>> I don't have an extensive experience with DV too, I just have samples > >>> here and there like you, but it seems we share the same information. > >>> Let see if someone else react and ask for keeping the current 1088 lines > >>> for DV stored height, but if nobody does, I think it should be okay. > >>> > >>>> Do you want me to add a comment line e.g. "obey 'including any decoder > >>>> scaling or padding' from SMPTE ST 377"? > >>> I am not a core developer and will let others give their feedback. My > >>> personal opinion is that the spec is supposed to be well known and does > >>> not require commenting, but that it would be interesting to explicit why > >>> we make a difference between DV and MPEG2/AVC. And >personally, I don't > >>> have the answer to this question. If nobody has one, maybe a comment > >>> could say "because this is the observed common practice". > >>> > >>> Nicolas > >> > >> Some weeks later now and no replies, maybe time to go on ? > >> I think the "case AV_CODEC_ID_DVVIDEO:" can be removed as discussed, fate > >> updated and that should be ok for everybody. > >> (Ideally, it could have been an opportunity to document why we have this > >> "DV exception", but I understand it is not very comfortable to write as > >> there is no meaningful reason, so forget about this, this won't hold up > >> the patch anyway) > >> For information, there was a long thread recently on ffmpeg-user about a > >> "bug" in dnxhd stored_height (will be fixed with your patch): > >> https://ffmpeg.org/pipermail/ffmpeg-user/2023-February/056111.html > > > > Will apply the patch in a couple of days unless somebody objects. If you want > > to change DV height (seems reasonable), please send a follow up patch with > > fate updates after that. > > Oh, this patch needs a fate update as well. On that note, DNXHD is a > macroblock-based codec, so why are we changing 1088 height to 1080? I > could ask the same for ProRes. The patch should explain better why those > need to be changed, if they do. FYI. The relationship between stored height/width and the corresponding Prores parameters (horizontal_size, vertical_size and bottomFieldVerticalSize) is specified in SMPTE RDD 44. > > Thanks, > Marton > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
On 10/03/2023 22:10, Marton Balint wrote: > > > On Mon, 6 Mar 2023, Nicolas Gaullier wrote: > > [...] >> >> Some weeks later now and no replies, maybe time to go on ? >> I think the "case AV_CODEC_ID_DVVIDEO:" can be removed as discussed, >> fate updated and that should be ok for everybody. >> (Ideally, it could have been an opportunity to document why we have >> this "DV exception", but I understand it is not very comfortable to >> write as there is no meaningful reason, so forget about this, this >> won't hold up the patch anyway) >> For information, there was a long thread recently on ffmpeg-user >> about a "bug" in dnxhd stored_height (will be fixed with your patch): >> https://ffmpeg.org/pipermail/ffmpeg-user/2023-February/056111.html > > Will apply the patch in a couple of days unless somebody objects. If > you want to change DV height (seems reasonable), please send a follow > up patch with fate updates after that. Apologizes for the huge delay. Attached is an updated patch. I changed the DV part (also removed from the 16x16 macroblock thing) I added some comments about specs (summary: DNxHD is explicit, others are not but implementation I know don't do the 16x16 macroblock thing). The FATE tests for DV are not impacted because they are SD and SD width/height are multiple of 16 so I added a DV100 test. Jérôme From c93c73164d427b2bcd29744b5a26ab82b1fe223a Mon Sep 17 00:00:00 2001 From: Jerome Martinez <jerome@mediaarea.net> Date: Sat, 14 Jan 2023 13:32:36 +0100 Subject: [PATCH] avformat/mxfenc: fix stored/sampled/displayed width/height Stored values are rounded to upper 16 multiple only for MPEG related formats (DV is not 16x16 macroblocks based, RDD 44 only refers to ST 377-1 without precision and no known implementation puts 1088 or 544, ST 2019-4 explicitly indicates 1080 for 1080p and 540 for 1080i) Sampled and displayed widths are codecpar ones (with DV exception) --- libavformat/mxfenc.c | 27 +++++++++++++++++++++------ tests/fate/lavf-container.mak | 3 ++- tests/ref/lavf/mxf_dvcpro100 | 3 +++ tests/ref/lavf/mxf_opatom | 2 +- 4 files changed, 27 insertions(+), 8 deletions(-) create mode 100644 tests/ref/lavf/mxf_dvcpro100 diff --git a/libavformat/mxfenc.c b/libavformat/mxfenc.c index 58c551c83c..614fc5ec89 100644 --- a/libavformat/mxfenc.c +++ b/libavformat/mxfenc.c @@ -1109,8 +1109,9 @@ static int64_t mxf_write_cdci_common(AVFormatContext *s, AVStream *st, const UID { MXFStreamContext *sc = st->priv_data; AVIOContext *pb = s->pb; - int stored_width = 0; - int stored_height = (st->codecpar->height+15)/16*16; + int stored_width = st->codecpar->width; + int stored_height = st->codecpar->height; + int display_width; int display_height; int f1, f2; const MXFCodecUL *color_primaries_ul; @@ -1129,12 +1130,24 @@ static int64_t mxf_write_cdci_common(AVFormatContext *s, AVStream *st, const UID else if (st->codecpar->height == 720) stored_width = 1280; } - if (!stored_width) - stored_width = (st->codecpar->width+15)/16*16; + display_width = stored_width; + switch (st->codecpar->codec_id) { + case AV_CODEC_ID_MPEG2VIDEO: + case AV_CODEC_ID_H264: + //Based on 16x16 macroblocks + stored_width = (stored_width+15)/16*16; + stored_height = (stored_height+15)/16*16; + break; + default: + break; + } + + //Stored width mxf_write_local_tag(s, 4, 0x3203); avio_wb32(pb, stored_width); + //Stored height mxf_write_local_tag(s, 4, 0x3202); avio_wb32(pb, stored_height>>sc->interlaced); @@ -1154,7 +1167,7 @@ static int64_t mxf_write_cdci_common(AVFormatContext *s, AVStream *st, const UID //Sampled width mxf_write_local_tag(s, 4, 0x3205); - avio_wb32(pb, stored_width); + avio_wb32(pb, display_width); //Samples height mxf_write_local_tag(s, 4, 0x3204); @@ -1168,8 +1181,9 @@ static int64_t mxf_write_cdci_common(AVFormatContext *s, AVStream *st, const UID mxf_write_local_tag(s, 4, 0x3207); avio_wb32(pb, 0); + //Display width mxf_write_local_tag(s, 4, 0x3209); - avio_wb32(pb, stored_width); + avio_wb32(pb, display_width); if (st->codecpar->height == 608) // PAL + VBI display_height = 576; @@ -1178,6 +1192,7 @@ static int64_t mxf_write_cdci_common(AVFormatContext *s, AVStream *st, const UID else display_height = st->codecpar->height; + //Display height mxf_write_local_tag(s, 4, 0x3208); avio_wb32(pb, display_height>>sc->interlaced); diff --git a/tests/fate/lavf-container.mak b/tests/fate/lavf-container.mak index 4bf7636b56..a04d31156a 100644 --- a/tests/fate/lavf-container.mak +++ b/tests/fate/lavf-container.mak @@ -8,7 +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 ENCDEC2, MPEG2VIDEO, PCM_S16LE, MXF) += mxf mxf_dv25 mxf_dvcpro50 +FATE_LAVF_CONTAINER-$(call ENCDEC2, MPEG2VIDEO, PCM_S16LE, MXF) += mxf mxf_dv25 mxf_dvcpro50 mxf_dvcpro100 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 FATE_LAVF_CONTAINER-$(call ENCDEC2, MPEG4, MP2, NUT) += nut @@ -55,6 +55,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_dvcpro100: CMD = lavf_container "-ar 48000 -ac 2" "-r 25 -vf scale=1440:1080,setdar=16/9 -c:v dvvideo -pix_fmt yuv422p -b 100000k -top 0 -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_dvcpro100 b/tests/ref/lavf/mxf_dvcpro100 new file mode 100644 index 0000000000..8193828e2c --- /dev/null +++ b/tests/ref/lavf/mxf_dvcpro100 @@ -0,0 +1,3 @@ +efa5cdde54ac38b02827ee8b6d7f03a4 *tests/data/lavf/lavf.mxf_dvcpro100 +14637613 tests/data/lavf/lavf.mxf_dvcpro100 +tests/data/lavf/lavf.mxf_dvcpro100 CRC=0x245e9a5f diff --git a/tests/ref/lavf/mxf_opatom b/tests/ref/lavf/mxf_opatom index 75f85b604e..fa72241813 100644 --- a/tests/ref/lavf/mxf_opatom +++ b/tests/ref/lavf/mxf_opatom @@ -1,3 +1,3 @@ -215ea72602bfeb70e99fc9d79fef073c *tests/data/lavf/lavf.mxf_opatom +6418766ca9b8b23fae74a80d66f26f3e *tests/data/lavf/lavf.mxf_opatom 4717625 tests/data/lavf/lavf.mxf_opatom tests/data/lavf/lavf.mxf_opatom CRC=0xb13ba2f8
On Tue, 14 Mar 2023, Jerome Martinez wrote: > On 10/03/2023 22:10, Marton Balint wrote: >> >> >> On Mon, 6 Mar 2023, Nicolas Gaullier wrote: >> >> [...] >>> >>> Some weeks later now and no replies, maybe time to go on ? >>> I think the "case AV_CODEC_ID_DVVIDEO:" can be removed as discussed, fate >>> updated and that should be ok for everybody. >>> (Ideally, it could have been an opportunity to document why we have this >>> "DV exception", but I understand it is not very comfortable to write as >>> there is no meaningful reason, so forget about this, this won't hold up >>> the patch anyway) >>> For information, there was a long thread recently on ffmpeg-user about a >>> "bug" in dnxhd stored_height (will be fixed with your patch): >>> https://ffmpeg.org/pipermail/ffmpeg-user/2023-February/056111.html >> >> Will apply the patch in a couple of days unless somebody objects. If you >> want to change DV height (seems reasonable), please send a follow up patch >> with fate updates after that. > > > Apologizes for the huge delay. > Attached is an updated patch. > > I changed the DV part (also removed from the 16x16 macroblock thing) > I added some comments about specs (summary: DNxHD is explicit, others are not > but implementation I know don't do the 16x16 macroblock thing). > The FATE tests for DV are not impacted because they are SD and SD > width/height are multiple of 16 so I added a DV100 test. Thanks, will apply with some extended commit description. Regards, Marton
diff --git a/libavformat/mxfenc.c b/libavformat/mxfenc.c index 58c551c83c..0b7e83ba4d 100644 --- a/libavformat/mxfenc.c +++ b/libavformat/mxfenc.c @@ -1109,8 +1109,9 @@ static int64_t mxf_write_cdci_common(AVFormatContext *s, AVStream *st, const UID { MXFStreamContext *sc = st->priv_data; AVIOContext *pb = s->pb; - int stored_width = 0; - int stored_height = (st->codecpar->height+15)/16*16; + int stored_width = st->codecpar->width; + int stored_height = st->codecpar->height; + int display_width; int display_height; int f1, f2; const MXFCodecUL *color_primaries_ul; @@ -1129,12 +1130,25 @@ static int64_t mxf_write_cdci_common(AVFormatContext *s, AVStream *st, const UID else if (st->codecpar->height == 720) stored_width = 1280; } - if (!stored_width) - stored_width = (st->codecpar->width+15)/16*16; + display_width = stored_width; + switch (st->codecpar->codec_id) { + case AV_CODEC_ID_MPEG2VIDEO: + case AV_CODEC_ID_DVVIDEO: + case AV_CODEC_ID_H264: + //Based on 16x16 macroblocks + stored_width = (stored_width+15)/16*16; + stored_height = (stored_height+15)/16*16; + break; + default: + break; + } + + //Stored width mxf_write_local_tag(s, 4, 0x3203); avio_wb32(pb, stored_width); + //Stored height mxf_write_local_tag(s, 4, 0x3202); avio_wb32(pb, stored_height>>sc->interlaced); @@ -1154,7 +1168,7 @@ static int64_t mxf_write_cdci_common(AVFormatContext *s, AVStream *st, const UID //Sampled width mxf_write_local_tag(s, 4, 0x3205); - avio_wb32(pb, stored_width); + avio_wb32(pb, display_width); //Samples height mxf_write_local_tag(s, 4, 0x3204); @@ -1168,8 +1182,9 @@ static int64_t mxf_write_cdci_common(AVFormatContext *s, AVStream *st, const UID mxf_write_local_tag(s, 4, 0x3207); avio_wb32(pb, 0); + //Display width mxf_write_local_tag(s, 4, 0x3209); - avio_wb32(pb, stored_width); + avio_wb32(pb, display_width); if (st->codecpar->height == 608) // PAL + VBI display_height = 576; @@ -1178,6 +1193,7 @@ static int64_t mxf_write_cdci_common(AVFormatContext *s, AVStream *st, const UID else display_height = st->codecpar->height; + //Display height mxf_write_local_tag(s, 4, 0x3208); avio_wb32(pb, display_height>>sc->interlaced);
Before the patch: - stored values were rounded to upper 16 multiple also for formats not using macroblocks (should be st->codecpar->width and st->codecpar->height when not MPEG formats; note that I found no other muxer doing the rounding for AVC, only for MPEG-2 Video, but I find no reason in specs for doing the difference so I kept the rounding for AVC) - sampled and displayed widths were stored width (should be st->codecpar->width like it is already done for height, with the DV50/100 exception) Could be tested with e.g. - fixed stored width (1912 instead of 1920) and height (1080 instead of 1088) not multiple of 16 : ffmpeg -f lavfi -i testsrc=duration=1:size=1912x1080 -c:v jpeg2000 test_prores.mxf - fixed sampled/displayed width (1912 instead of 1920): ffmpeg -f lavfi -i testsrc=duration=1:size=1912x1080 -c:v mpeg2video test_mpeg2video.mxf From cda353059886182aab2e258023c4d027c448344b Mon Sep 17 00:00:00 2001 From: Jerome Martinez <jerome@mediaarea.net> Date: Sat, 14 Jan 2023 13:32:36 +0100 Subject: [PATCH] avformat/mxfenc: fix stored/sampled/displayed width/height Stored values are rounded to upper 16 multiple only for MPEG related formats Sampled and displayed widths are codecpar ones (with DV exception) --- libavformat/mxfenc.c | 28 ++++++++++++++++++++++------ 1 file changed, 22 insertions(+), 6 deletions(-)