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

[Dave Rice]

> On Jan 20, 2023, at 10:17 AM, Tomas Härdin <git at haerdin.se> wrote:
> 
> 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.

I'm just nudging on the consideration of merging this patch. I've been testing it over the last week with ffv1/mxf content and have found this demuxing support very helpful.
Dave Rice