[FFmpeg-devel] [PATCH][RFC] avutil/spherical: add a flag to signal tiled/padded projections

[James Almer]

On 3/30/2017 11:41 AM, Aaron Colwell wrote:
> On Thu, Mar 30, 2017 at 7:17 AM James Almer <jamrial at gmail.com> wrote:
> 
>> On 3/30/2017 5:54 AM, Vittorio Giovara wrote:
>>> On Wed, Mar 29, 2017 at 8:01 PM, James Almer <jamrial at gmail.com> wrote:
>>>> A new field is added to AVSphericalMapping for this purpose,
>>>> and is used by both Equirectangular and Cubemap projections.
>>>> This is a replacement for duplicate projection enums like
>>>> AV_SPHERICAL_EQUIRECTANGULAR_TILE, which are therefore
>>>> removed.
>>>>
>>>> Signed-off-by: James Almer <jamrial at gmail.com>
>>>> ---
>>>> This patch depends on the av_spherical_projection_name() patchset for
>>>> simplicity purposes.
>>>>
>>>> Ok, this is an RFC mainly because of the API/ABI break it represents.
>>>> The AV_SPHERICAL_EQUIRECTANGULAR_TILE projection is a month and a half
>>>> old and not present in any release, plus a major bump is queued as part
>>>> of the merges, so i personally think this change is acceptable as is for
>>>> such an niche and recent feature.
>>>> If not then i can deprecate said projection enum value instead and keep
>>>> the current muxer functionality for it for a while. It will need a lot
>>>> of preprocessor guards, though.
>>>>
>>>> The reason for this change is that eventually, a new projection enum
>>>> for padded cubemap may be suggested with the same arguments as the ones
>>>> used to introduce the one for tiled equirectangular. Then if any new
>>>> real projections are added, we'd could end up with duplicate enums for
>>>> them as well, when setting a single shared flag would be enough.
>>>> Stereo3D avoided a lot of duplicate types with the inverted flag, so i
>>>> figured the same should be done here.
>>>>
>>>> Improved doxy and/or flag name is extremely welcome (Read: needed).
>>>
>>> I'm against this idea because of explanations given in other threads.
>>>
>>> The stereo3d case is different because it's a property that can be
>>> applied to all types, while the cropped/padded feature applies to
>>> current existing projections, not the future ones.
>>
>> But it could apply. It's fairly likely that cropping/padding of unused
>> pixels will be present in future projections.
>> And this flag will not necessarily be the only one in the long term.
>> You could end up having a new property at some point that may have to
>> be signaled in some way. And it could even be present in a projection
>> alongside cropping/padding.
>>
>>
> It probably isn't a surprise, but I agree with James here. I think it is
> highly likely other projections will have tiling/cropping in them and I
> think it is better to reflect this as a flag instead of merging it with the
> projection enum.
> 
> I'm not 100% sure padding should be rolled into the same flag as cropping,
> but I think that is a detail that could be ironed out separately. I think
> the flag mechanism itself is a good idea.

Be it single or separate flags it's fine by me. My intention is indeed to
introduce the flag mechanism to easily signal the presence of projection
specific properties while keeping the enums to one per projection.

> 
> 
>>> Additionally there
>>> is nothing wrong with having more enum values, since it is likely that
>>> future cubemaps layouts will have a different enum value, and not
>>> another field to check.
>>
>> Having a single flags field with dozens of potential flags seems like
>> a much cleaner solution than several enum values to list the many
>> different ways a single projection can be handled to me.
>>
> 
> I agree. In my view this addresses the "having to check multiple fields"
> objection raised in previous threads because now a single flag can be used
> to indicate whether you need to pay attention to the bound_xxx fields or
> not. It seems like a more reasonable and extendable compromise to me.
> 
> I also don't think different cubemap layouts necessarily should have their
> own enum values. This feels roughly akin to merging an audio codec_id and
> the channel mapping into a single enum space.
> 
> Aaron

CCing Vittorio so he may see your reply as well.