[FFmpeg-devel] avformat/mxfenc: fix stored/sampled/displayed width/height

[Marton Balint]

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