| Message ID | 20170329180124.7544-1-jamrial@gmail.com |
|---|---|
| State | New |
| Headers | show |
On Wed, Mar 29, 2017 at 8:01 PM, James Almer <jamrial@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@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. 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. Regardless of the outcome of the discussion, please keep the API aligned for users' sake.
On Wed, Mar 29, 2017 at 03:01:24PM -0300, James Almer 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@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. If you plan to still work on the API/ABI of this, it would make sense to mark it as experimental until such work is fully done. That would avoid any API/ABI compatibility issues [...]
On 3/30/2017 5:54 AM, Vittorio Giovara wrote: > On Wed, Mar 29, 2017 at 8:01 PM, James Almer <jamrial@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@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. > 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. > > Regardless of the outcome of the discussion, please keep the API > aligned for users' sake. If you mean between projects then I have no issues submitting this to libav, regardless of the choice of removal or deprecation.
On Thu, Mar 30, 2017 at 7:17 AM James Almer <jamrial@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@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@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. > > 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
On 3/30/2017 11:41 AM, Aaron Colwell wrote: > On Thu, Mar 30, 2017 at 7:17 AM James Almer <jamrial@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@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@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.
On Thu, Mar 30, 2017 at 4:16 PM, James Almer <jamrial@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@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@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).
On 3/30/2017 3:37 PM, Vittorio Giovara wrote: > On Thu, Mar 30, 2017 at 4:16 PM, James Almer <jamrial@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@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@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?
On Thu, Mar 30, 2017 at 9:52 PM, James Almer <jamrial@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@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@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@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.
diff --git a/doc/APIchanges b/doc/APIchanges index f600777072..7453e80711 100644 --- a/doc/APIchanges +++ b/doc/APIchanges @@ -16,6 +16,10 @@ libavutil: 2015-08-28 API changes, most recent first: 2017-xx-xx - xxxxxxx - lavu 55.xx.xxx - spherical.h + Add flags field to AVSphericalMapping and AV_SPHERICAL_FLAG_CROPPED flag. + Remove AV_SPHERICAL_EQUIRECTANGULAR_TILE AVSphericalProjection. + +2017-xx-xx - xxxxxxx - lavu 55.xx.xxx - spherical.h Add av_spherical_projection_name() 2017-03-xx - xxxxxxx - lavu 55.52.100 - avutil.h diff --git a/ffprobe.c b/ffprobe.c index ad3b77d74d..8e242ed680 100644 --- a/ffprobe.c +++ b/ffprobe.c @@ -1874,11 +1874,13 @@ static void print_pkt_side_data(WriterContext *w, const AVSphericalMapping *spherical = (AVSphericalMapping *)sd->data; print_str("projection", av_spherical_projection_name(spherical->projection)); if (spherical->projection == AV_SPHERICAL_CUBEMAP) { + print_int("padded", !!(spherical->flags & AV_SPHERICAL_FLAG_CROPPED)); print_int("padding", spherical->padding); - } else if (spherical->projection == AV_SPHERICAL_EQUIRECTANGULAR_TILE) { + } else if (spherical->projection == AV_SPHERICAL_EQUIRECTANGULAR) { size_t l, t, r, b; av_spherical_tile_bounds(spherical, par->width, par->height, &l, &t, &r, &b); + print_int("tiled", !!(spherical->flags & AV_SPHERICAL_FLAG_CROPPED)); print_int("bound_left", l); print_int("bound_top", t); print_int("bound_right", r); diff --git a/libavformat/dump.c b/libavformat/dump.c index e27d1b9422..37b46ae305 100644 --- a/libavformat/dump.c +++ b/libavformat/dump.c @@ -356,7 +356,6 @@ static void dump_spherical(void *ctx, AVCodecParameters *par, AVPacketSideData * av_log(ctx, AV_LOG_INFO, "%s", av_spherical_projection_name(spherical->projection)); if (spherical->projection != AV_SPHERICAL_EQUIRECTANGULAR && - spherical->projection != AV_SPHERICAL_EQUIRECTANGULAR_TILE && spherical->projection != AV_SPHERICAL_CUBEMAP) { return; } @@ -366,7 +365,10 @@ static void dump_spherical(void *ctx, AVCodecParameters *par, AVPacketSideData * roll = ((double)spherical->roll) / (1 << 16); av_log(ctx, AV_LOG_INFO, " (%f/%f/%f) ", yaw, pitch, roll); - if (spherical->projection == AV_SPHERICAL_EQUIRECTANGULAR_TILE) { + if (!(spherical->flags & AV_SPHERICAL_FLAG_CROPPED)) + return; + + if (spherical->projection == AV_SPHERICAL_EQUIRECTANGULAR) { size_t l, t, r, b; av_spherical_tile_bounds(spherical, par->width, par->height, &l, &t, &r, &b); diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c index 2e2a814b4f..9e1b5f8d45 100644 --- a/libavformat/matroskadec.c +++ b/libavformat/matroskadec.c @@ -1915,7 +1915,7 @@ static int mkv_parse_video_projection(AVStream *st, const MatroskaTrack *track) size_t spherical_size; uint32_t l = 0, t = 0, r = 0, b = 0; uint32_t padding = 0; - int ret; + int ret, flags = 0; GetByteContext gb; bytestream2_init(&gb, track->video.projection.private.data, @@ -1930,6 +1930,7 @@ static int mkv_parse_video_projection(AVStream *st, const MatroskaTrack *track) switch (track->video.projection.type) { case MATROSKA_VIDEO_PROJECTION_TYPE_EQUIRECTANGULAR: + projection = AV_SPHERICAL_EQUIRECTANGULAR; if (track->video.projection.private.size == 20) { t = bytestream2_get_be32(&gb); b = bytestream2_get_be32(&gb); @@ -1949,9 +1950,7 @@ static int mkv_parse_video_projection(AVStream *st, const MatroskaTrack *track) } if (l || t || r || b) - projection = AV_SPHERICAL_EQUIRECTANGULAR_TILE; - else - projection = AV_SPHERICAL_EQUIRECTANGULAR; + flags |= AV_SPHERICAL_FLAG_CROPPED; break; case MATROSKA_VIDEO_PROJECTION_TYPE_CUBEMAP: if (track->video.projection.private.size < 4) { @@ -1966,6 +1965,9 @@ static int mkv_parse_video_projection(AVStream *st, const MatroskaTrack *track) } projection = AV_SPHERICAL_CUBEMAP; padding = bytestream2_get_be32(&gb); + + if (padding) + flags |= AV_SPHERICAL_FLAG_CROPPED; } else { av_log(NULL, AV_LOG_ERROR, "Unknown spherical metadata\n"); return AVERROR_INVALIDDATA; @@ -1979,6 +1981,7 @@ static int mkv_parse_video_projection(AVStream *st, const MatroskaTrack *track) if (!spherical) return AVERROR(ENOMEM); spherical->projection = projection; + spherical->flags = flags; spherical->yaw = (int32_t)(track->video.projection.yaw * (1 << 16)); spherical->pitch = (int32_t)(track->video.projection.pitch * (1 << 16)); diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c index af941ceb8f..0adbe3b46b 100644 --- a/libavformat/matroskaenc.c +++ b/libavformat/matroskaenc.c @@ -936,17 +936,15 @@ static int mkv_write_video_projection(AVFormatContext *s, AVIOContext *pb, AVStr switch (spherical->projection) { case AV_SPHERICAL_EQUIRECTANGULAR: - put_ebml_uint(dyn_cp, MATROSKA_ID_VIDEOPROJECTIONTYPE, - MATROSKA_VIDEO_PROJECTION_TYPE_EQUIRECTANGULAR); - break; - case AV_SPHERICAL_EQUIRECTANGULAR_TILE: { AVIOContext b; uint8_t private[20]; - ffio_init_context(&b, private, sizeof(private), - 1, NULL, NULL, NULL, NULL); put_ebml_uint(dyn_cp, MATROSKA_ID_VIDEOPROJECTIONTYPE, MATROSKA_VIDEO_PROJECTION_TYPE_EQUIRECTANGULAR); + if (!(spherical->flags & AV_SPHERICAL_FLAG_CROPPED)) + break; + ffio_init_context(&b, private, sizeof(private), + 1, NULL, NULL, NULL, NULL); avio_wb32(&b, 0); // version + flags avio_wb32(&b, spherical->bound_top); avio_wb32(&b, spherical->bound_bottom); diff --git a/libavformat/mov.c b/libavformat/mov.c index 127aec1d1d..3bbf3507d1 100644 --- a/libavformat/mov.c +++ b/libavformat/mov.c @@ -4634,7 +4634,7 @@ static int mov_read_sv3d(MOVContext *c, AVIOContext *pb, MOVAtom atom) { AVStream *st; MOVStreamContext *sc; - int size, layout; + int size, layout, flags = 0; int32_t yaw, pitch, roll; uint32_t l = 0, t = 0, r = 0, b = 0; uint32_t tag, padding = 0; @@ -4705,6 +4705,8 @@ static int mov_read_sv3d(MOVContext *c, AVIOContext *pb, MOVAtom atom) } projection = AV_SPHERICAL_CUBEMAP; padding = avio_rb32(pb); + if (padding) + flags |= AV_SPHERICAL_FLAG_CROPPED; break; case MKTAG('e','q','u','i'): t = avio_rb32(pb); @@ -4719,10 +4721,9 @@ static int mov_read_sv3d(MOVContext *c, AVIOContext *pb, MOVAtom atom) return AVERROR_INVALIDDATA; } + projection = AV_SPHERICAL_EQUIRECTANGULAR; if (l || t || r || b) - projection = AV_SPHERICAL_EQUIRECTANGULAR_TILE; - else - projection = AV_SPHERICAL_EQUIRECTANGULAR; + flags |= AV_SPHERICAL_FLAG_CROPPED; break; default: av_log(c->fc, AV_LOG_ERROR, "Unknown projection type\n"); @@ -4734,6 +4735,7 @@ static int mov_read_sv3d(MOVContext *c, AVIOContext *pb, MOVAtom atom) return AVERROR(ENOMEM); sc->spherical->projection = projection; + sc->spherical->flags = flags; sc->spherical->yaw = yaw; sc->spherical->pitch = pitch; diff --git a/libavformat/movenc.c b/libavformat/movenc.c index a54aa879e9..fd96701d11 100644 --- a/libavformat/movenc.c +++ b/libavformat/movenc.c @@ -1640,7 +1640,6 @@ static int mov_write_sv3d_tag(AVFormatContext *s, AVIOContext *pb, AVSphericalMa const char* metadata_source = s->flags & AVFMT_FLAG_BITEXACT ? "Lavf" : LIBAVFORMAT_IDENT; if (spherical_mapping->projection != AV_SPHERICAL_EQUIRECTANGULAR && - spherical_mapping->projection != AV_SPHERICAL_EQUIRECTANGULAR_TILE && spherical_mapping->projection != AV_SPHERICAL_CUBEMAP) { av_log(pb, AV_LOG_WARNING, "Unsupported projection %d. sv3d not written.\n", spherical_mapping->projection); return 0; @@ -1670,7 +1669,6 @@ static int mov_write_sv3d_tag(AVFormatContext *s, AVIOContext *pb, AVSphericalMa switch (spherical_mapping->projection) { case AV_SPHERICAL_EQUIRECTANGULAR: - case AV_SPHERICAL_EQUIRECTANGULAR_TILE: avio_wb32(pb, 28); /* size */ ffio_wfourcc(pb, "equi"); avio_wb32(pb, 0); /* version = 0 & flags = 0 */ diff --git a/libavutil/spherical.c b/libavutil/spherical.c index 1d06e7c552..29a386787e 100644 --- a/libavutil/spherical.c +++ b/libavutil/spherical.c @@ -54,8 +54,6 @@ void av_spherical_tile_bounds(const AVSphericalMapping *map, static const char *spherical_projection_names[] = { [AV_SPHERICAL_EQUIRECTANGULAR] = "equirectangular", [AV_SPHERICAL_CUBEMAP] = "cubemap", - [AV_SPHERICAL_EQUIRECTANGULAR_TILE] = "tiled equirectangular", - }; const char *av_spherical_projection_name(enum AVSphericalProjection projection) diff --git a/libavutil/spherical.h b/libavutil/spherical.h index 2c8dd3cd97..e628c3621a 100644 --- a/libavutil/spherical.h +++ b/libavutil/spherical.h @@ -63,16 +63,14 @@ enum AVSphericalProjection { * to the back. */ AV_SPHERICAL_CUBEMAP, - - /** - * Video represents a portion of a sphere mapped on a flat surface - * using equirectangular projection. The @ref bounding fields indicate - * the position of the current video in a larger surface. - */ - AV_SPHERICAL_EQUIRECTANGULAR_TILE, }; /** + * Projection has padding/cropping + */ +#define AV_SPHERICAL_FLAG_CROPPED (1 << 0) + +/** * This structure describes how to handle spherical videos, outlining * information about projection, initial layout, and any other view modifier. * @@ -160,9 +158,10 @@ typedef struct AVSphericalMapping { * original_height = tile->height + bound_top + bound_bottom; * @endcode * - * @note These values are valid only for the tiled equirectangular - * projection type (@ref AV_SPHERICAL_EQUIRECTANGULAR_TILE), - * and should be ignored in all other cases. + * @note These values are valid only for the equirectangular projection + * type (@ref AV_SPHERICAL_EQUIRECTANGULAR) when the + * (@ref AV_SPHERICAL_FLAG_CROPPED) flag is set, and should be + * ignored in all other cases. */ uint32_t bound_left; ///< Distance from the left edge uint32_t bound_top; ///< Distance from the top edge @@ -176,10 +175,15 @@ typedef struct AVSphericalMapping { * Number of pixels to pad from the edge of each cube face. * * @note This value is valid for only for the cubemap projection type - * (@ref AV_SPHERICAL_CUBEMAP), and should be ignored in all other - * cases. + * (@ref AV_SPHERICAL_CUBEMAP) when the (@ref AV_SPHERICAL_FLAG_CROPPED) + * flag is set, and should be ignored in all other cases. */ uint32_t padding; + + /** + * Additional information about the projection. + */ + int flags; } AVSphericalMapping; /** diff --git a/tests/ref/fate/matroska-spherical-mono b/tests/ref/fate/matroska-spherical-mono index bd57d94514..078c7f5248 100644 --- a/tests/ref/fate/matroska-spherical-mono +++ b/tests/ref/fate/matroska-spherical-mono @@ -6,7 +6,8 @@ inverted=0 [/SIDE_DATA] [SIDE_DATA] side_data_type=Spherical Mapping -projection=tiled equirectangular +projection=equirectangular +tiled=1 bound_left=148 bound_top=73 bound_right=147 diff --git a/tests/ref/fate/mov-spherical-mono b/tests/ref/fate/mov-spherical-mono index bd57d94514..078c7f5248 100644 --- a/tests/ref/fate/mov-spherical-mono +++ b/tests/ref/fate/mov-spherical-mono @@ -6,7 +6,8 @@ inverted=0 [/SIDE_DATA] [SIDE_DATA] side_data_type=Spherical Mapping -projection=tiled equirectangular +projection=equirectangular +tiled=1 bound_left=148 bound_top=73 bound_right=147
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@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). doc/APIchanges | 4 ++++ ffprobe.c | 4 +++- libavformat/dump.c | 6 ++++-- libavformat/matroskadec.c | 11 +++++++---- libavformat/matroskaenc.c | 10 ++++------ libavformat/mov.c | 10 ++++++---- libavformat/movenc.c | 2 -- libavutil/spherical.c | 2 -- libavutil/spherical.h | 28 ++++++++++++++++------------ tests/ref/fate/matroska-spherical-mono | 3 ++- tests/ref/fate/mov-spherical-mono | 3 ++- 11 files changed, 48 insertions(+), 35 deletions(-)