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

[Pierre-Anthony Lemieux]

On Mon, Mar 13, 2023 at 3:30 PM Marton Balint <cus at 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 at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".