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

Vittorio Giovara vittorio.giovara at gmail.com
Fri Mar 31 11:35:43 EEST 2017


On Thu, Mar 30, 2017 at 9:52 PM, James Almer <jamrial at gmail.com> wrote:
> 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.

All I'm saying is that we shouldn't pre-emptively change the API
because it might be useful in the future. Let's change it when there
is a real need for it.

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

Of course.
-- 
Vittorio


More information about the ffmpeg-devel mailing list