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

[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.

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.

Jérôme