On 31/01/2023 15:53, Tomas Härdin wrote:
> sön 2023-01-29 klockan 11:36 -0500 skrev Dave Rice:
>>
>>> On Jan 20, 2023, at 10:17 AM, Tomas Härdin <git@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.
> Surely you mean muxing?
>
> Some FATE tests would be nice.


Apologizes for the huge delay.
Attached is an updated patch with a FATE test.

Jérôme