[FFmpeg-devel] avformat/mxfenc: SMPTE RDD 48:2018 Amd 1:2022 (FFV1 in MXF) support

[Tomas Härdin]

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