On Thu, Mar 30, 2017 at 9:52 PM, James Almer <jamr...@gmail.com> wrote: > On 3/30/2017 3:37 PM, Vittorio Giovara wrote: >> On Thu, Mar 30, 2017 at 4:16 PM, James Almer <jamr...@gmail.com> wrote: >>> On 3/30/2017 5:54 AM, Vittorio Giovara wrote: >>>> On Wed, Mar 29, 2017 at 8:01 PM, James Almer <jamr...@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 <jamr...@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 _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
