[FFmpeg-devel] [PATCH v2 1/3] libaomenc: Add support for tiles

[Mark Thompson]

On 19/09/18 00:15, James Almer wrote:
> On 9/18/2018 7:55 PM, Mark Thompson wrote:
>> On 18/09/18 01:12, James Almer wrote:
>>> On 9/17/2018 8:47 PM, Mark Thompson wrote:
>>>> Adds an option to specify the number of tile rows and columns, then uses
>>>> equal-sized tiles to fill the frame.
>>>> ---
>>>>  libavcodec/libaomenc.c | 54 ++++++++++++++++++++++++++++++++++++++++++
>>>>  1 file changed, 54 insertions(+)
>>>>
>>>> diff --git a/libavcodec/libaomenc.c b/libavcodec/libaomenc.c
>>>> index 6a79d9b873..3ccff0e0fb 100644
>>>> --- a/libavcodec/libaomenc.c
>>>> +++ b/libavcodec/libaomenc.c
>>>> ...
>>>> @@ -742,6 +795,7 @@ static const AVOption options[] = {
>>>>      { "static-thresh",    "A change threshold on blocks below which they will be skipped by the encoder", OFFSET(static_thresh), AV_OPT_TYPE_INT, { .i64 = 0 }, 0, INT_MAX, VE },
>>>>      { "drop-threshold",   "Frame drop threshold", offsetof(AOMContext, drop_threshold), AV_OPT_TYPE_INT, {.i64 = 0 }, INT_MIN, INT_MAX, VE },
>>>>      { "noise-sensitivity", "Noise sensitivity", OFFSET(noise_sensitivity), AV_OPT_TYPE_INT, {.i64 = 0 }, 0, 4, VE},
>>>> +    { "tiles",            "Tile rows x columns", OFFSET(tile_cols), AV_OPT_TYPE_IMAGE_SIZE, { .str = NULL }, 0, 0, VE },
>>>
>>> Using separate tile-columns and tile-rows AV_OPT_TYPE_INT options would
>>> be more consistent with the libvpx wrapper, which already has them
>>> called like that and also shares a lot of other option names with the
>>> libaom.
>>
>> The options on libvpx-vp9 are actually log2 of the value, so "-tile-rows 3 -tile-columns 2" gives you 8x4 tiles.  (VP9 requires that the number of tiles in each dimension is a power of two, while AV1 lets you set arbitrary sizes.)
>>
>> I don't really mind how this works - I just thought the IMAGE_SIZE method looked nicer.  What do you prefer?
> 
> I usually prefer consistency in options for similar modules, but the
> equivalent of the VP9 options would be to set the AV1E_SET_TILE_* codec
> control IDs instead of what you're doing here, so your IMAGE_SIZE method
> is fine.
> 
> There's for that matter a conflicting patch called "lavc/libaomenc: Add
> -tile-columns/-tile-rows" by Kagami Hiiragi that sets the aforementioned
> codec control IDs with the same option names as the VP9 ones. Maybe both
> patches can be applied, and have the encoder abort if they are used at
> the same time? Otherwise apply yours alone since it allows arbitrary sizes.

I think having both would make sense - the two different options are reflecting the different values of uniform_tile_spacing_flag.  The explicit sizing can get the same result with the flag off as it being on, but will require more bits in every frame header to do so.

On matching behaviour with libvpx, I don't think tile_cols/rows_log2 in VP9 and AV1 (with uniform_tile_spacing_flag = 1) actually do have the same effect - VP9 ensures that you get exactly 2^tile_cols_log2 tile columns, while for AV1 it's an upper bound and depends on the width.  For example, given a width of 576 (as 9 64x64 superblocks) and tile_cols_log2 = 2, VP9 looks like it gives you { 2, 2, 2, 3 } while AV1 will give you { 3, 3, 3 } as the tile column widths.  That probably wants to be mentioned in the documentation, and maybe merits the options not matching precisely.

So, I'll look into supporting both cases in some consistent way.

Thanks,

- Mark