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

Vittorio Giovara vittorio.giovara at gmail.com
Thu Mar 30 21:37:48 EEST 2017


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.

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

Like... what?

> 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.

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).
-- 
Vittorio


More information about the ffmpeg-devel mailing list