[FFmpeg-devel] [PATCH] libavutil: add clean aperture (CLAP) side data.

[James Almer]

On 5/6/2020 12:22 PM, Neil Birkbeck wrote:
> On Tue, May 5, 2020 at 5:11 AM Kieran O Leary <kieran.o.leary at gmail.com>
> wrote:
> 
>> Hi,
>>
>> I broke the threading with my last reply, i apologise. Here goes another
>> attempt:
>>
>> On Tue, Apr 28, 2020 at 6:23 PM Neil Birkbeck <neil.birkbeck at gmail.com>
>> wrote:
>>
>>> On Tue, Apr 28, 2020 at 3:18 AM Nicolas George <george at nsup.org> wrote:
>>>
>>>> Andreas Rheinhardt (12020-04-28):
>>>>> That's expected. The patch provided only provides the structure in
>>> which
>>>>> the values are intended to be exported; it does not add any demuxing
>> or
>>>>> muxing capabilities for mov or mkv (as you can see from the fact that
>>>>> none of these (de)muxers have been changed in the patch).
>>>>
>>>> Which is something I intended to comment on: adding code without users
>>>> is rarely a good idea. I suggest we do not commit until at least one
>>>> demuxer use it, preferably at least two. Otherwise, we may realize that
>>>> “oh crap, it doesn't work” because of a tiny unforeseen detail.
>>>
>>>
>>> Thanks for the feedback. I also have patches for the demux (MOV/MKV) and
>>> mux (MOV/MKV).
>>>
>>> As there is still the alternative of using the fields in the
>>> AVCodecParameters/AVCodecContext, my intention was to keep the first
>> patch
>>> small to resolve discussion on that point.
>>>
>>> I've included the patches, if you'd like to try test it, Kieren. I see on
>>> your test file that there may be some slight rounding error making output
>>> crop 704 not 703 (MKV file ends up with pixel_crop_{left,right} = 8).
>>>
>>> /ffprobe ../testdata/clap.mov 2>&1 | grep -A1 "Side"
>>>     Side data:
>>>       Clean aperture:[width 41472/59 height:576/1 h_offset:0/1
>>> v_offset:0/1]
>>> ./ffmpeg -i ../testdata/clap.mov  -vcodec copy -acodec copy /tmp/clap.mkv
>>> ./ffprobe /tmp/clap.mkv 2>&1 | grep -A1 "Side"
>>>     Side data:
>>>       Clean aperture:[width 704/1 height:576/1 h_offset:0/1 v_offset:0/1]
>>>
>>
>> I have to look deeper into the MKV side of things and most likely raise it
>> with the cellar mailing list so that better minds than mine can weigh in. I
>> do see that the rounding up to 704 could be an issue alright.
>> As for MOV, your patch appears to generate the same output clap values as
>> the input, so that's really great! command line and mediainfo trace below:
>>
> 
> Thanks for testing, Kieran and for linking the discussion on the cellar
> list.
> 
> Any additional thoughts from ffmpeg devs on container-level SideData vs
> adding the extra fields into AVCodecParameters/AVCodecContext and plumbing
> into the frame instead? I anticipate some situations where there can be
> interaction between cropping in bitstream and container-level cropping.
> Maybe the best way forward is for me to share some sample patches for the
> alternative to validate whether it supports the various use cases
> (transmux, decode+crop, any other application level handling), and to
> confirm the interface changes for the structs.

One option could be to also introduce a frame side data for this new
struct and have it replace the AVFrame fields, which would then be
deprecated (But of course keep working until removed).
This would allow to either inject stream side data from clap atoms and
Matroska crop fields into packets (Which would then be propagated to
frames to be applied during decoding), or use and export the bitstream
cropping information as it's the case right now, all while preventing
the addition of new fields to AVCodecParameters.

I would like a developer that makes use of this feature to also comment,
especially seeing how the AVFrame fields and this clap side data are
pretty different.