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

James Almer jamrial at gmail.com
Thu Mar 30 22:52:49 EEST 2017


On 3/30/2017 3:37 PM, Vittorio Giovara wrote:
> On Thu, Mar 30, 2017 at 4:16 PM, 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.
> 
> I don't think so, there is a new projection in the works (mesh) and
> there is no such concept.

Aaron already said it's entirely possible, so i'm going to side with
the one that's writing the spec.

> 
>> And this flag will not necessarily be the only one in the long term.
> 
> Like... what?

Like the same things you argue should instead be signaled as their
own duplicate enum.
This question is really weird from your part. In one paragraph you
argue the current implementation is already extensible, then in
another you wonder why even think about being future proof.

> 
>> 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.
> 
> Let's not overengineer a simple API just because "at some point" we
> "may have to" signal something in the future.
> Right now the API is very simple (just check one field) and very
> extensible (just add an enum value), complicating it for "maybe we
> will need something" is not the way to go in my opinion.
> 
>>> 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 haven't found a single convincing argument for breaking the API,
> except "it's early", "it's a niche feature", and "maybe we'll need it
> in the future". So I'm sorry James, but I don't agree with this
> change.

I said I'm ok with deprecating the enum and adding compat code for it.
I'm not trying to break API/ABI, i'm trying to improve it and extend
it.
Those arguments were to support not bothering with deprecation, not
to support the addition of the flags field.

It seems to me that you're more annoyed and displeased at the API/ABI
breaking arguments, seeing you keep quoting them in every one of your
paragraphs, than those about the flags mechanism.

> 
> On 3/30/2017 11:41 AM, Aaron Colwell wrote:
>> 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.
> 
> The point is to avoid checking anything at all, except the main enum value.
> 
>> 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.
> 
> I disagree, this is similar as the tiled equirectangular case: right
> now applications check a single value and they know that they have to
> render the projection in a well known way. In case we add new cubemap
> layouts, we can't expect applications to keep reading this value *and*
> interpret a new field (be it a separate layout field or a flag
> variable) in addition to that. Users need to know what they can
> support and what not, and using a single enum field to signal this is
> the most immediate and simplest way (especially for this "niche"
> feature).

Guess i really touched a nerve there with my choice of words when
suggesting breaking the API.

I'm not going to keep arguing about this. If you'd rather add
duplicate enums for every slight difference in a given projection
that will require reading specific fields then this patch can be
dropped. Changing it to use flags can be done at any point i guess,
if for whatever reason you change your mind.

Could you at least review the other patchset so i may push it?



More information about the ffmpeg-devel mailing list