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

[Marton Balint]

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