[FFmpeg-devel] [PATCH 1/2 v2] avutil: add a Tile Grid API

[James Almer]

On 1/22/2024 7:32 AM, Anton Khirnov wrote:
> Quoting James Almer (2024-01-21 20:29:58)
>> On 1/21/2024 4:02 PM, Anton Khirnov wrote:
>>> I still don't see why should it be a good idea to use this struct for
>>> generic container cropping. It feels very much like a hammer in search
>>> of a nail.
>>
>> Because once we support container cropping, we will be defining a
>> stream/packet side data type that will contain a subset of the fields
>> from this struct.
> 
> AVCodecParameters is a subset of AVCodecContext. By this logic we should
> use AVCodecContext everywhere instead of AVCodecParameters.
> 
> There are also issues with storing it in packet side data since it
> can contain pointers to malloced data.

Ah, that's true. At least if i remove the union.

> 
>> If we reuse this struct, we can export a clap box as an AVTileGrid (Or i
>> can rename it to AVImageGrid, and tile to subrectangle) either as the
>> stream group tile grid specific parameters if HEIF, or as stream side
>> data otherwise.
> 
> How does that make the API better? It seems highly counterintuitive to
> me to export container cropping information in a struct designed for
> tiling.

Or you could consider it a struct designed to represent a group of 
images combined in a grid, for the purpose of extracting a subrectangle 
for presentation. How many images there are is defined by the rows and 
cols fields, and it can be 1.

> 
>>>
>>>>>
>>>>>> And what do you mean with not supporting describing arbitrary
>>>>>> partitioning? Isn't that what variable tile dimensions achieve?
>>>>>
>>>>> IIUC your tiling scheme still assumes that the partitioning is by rows
>>>>> and columns. A completely generic partitioning could be irregular.
>>>>
>>>> A new tile type that doesn't define rows and columns can be added if
>>>> needed. But the current variable tile type can support things like grids
>>>> of two rows and two columns where the second row is effectively a single
>>>> tile, simply by setting the second tile in said row as having a width of 0.
>>>
>>> The problem I see here is that every consumer of this struct then has to
>>> explicitly support every type, and adding a new type requires updating
>>> all callers. This seems unnecessary when "list of N rectangles" covers
>>> all possible partitionings.
>>
>> Well, the variable type supports a list of N rectangles where each
>> rectangle has arbitrary dimensions, and you can do things like having
>> three tiles/rectangles that together still form a rectangle, while
>> defining row and column count. So i don't personally see the need for a
>> new type to begin with.
> 
> I don't see how is that supposed to work. E.g. consider the following
> partitioning:
> ┌─┬────┬─┐
> │ │    ├─┤
> ├─┤    │ │
> │ ├────┤ │
> └─┴────┴─┘
> 
> How would you represent it in this API?
>