[1/3,v2] avutil/spherical: add functions to retrieve and request projection names

Message ID 20170407140914.4968-1-jamrial@gmail.com
State New
Headers show

Commit Message

James Almer April 7, 2017, 2:09 p.m.
Signed-off-by: James Almer <jamrial@gmail.com>
---
 doc/APIchanges        |  4 ++++
 libavutil/spherical.c | 28 ++++++++++++++++++++++++++++
 libavutil/spherical.h | 24 ++++++++++++++++++++++++
 libavutil/version.h   |  2 +-
 4 files changed, 57 insertions(+), 1 deletion(-)

Comments

Luca Barbato April 7, 2017, 2:22 p.m. | #1
On 07/04/2017 16:09, James Almer wrote:
> +int av_spherical_from_name(const char *name);

It can directly output the enum now, it is fine both ways to me.

Thank you for amending it :)

lu
Vittorio Giovara April 10, 2017, 1:56 p.m. | #2
On Fri, Apr 7, 2017 at 10:22 AM, Luca Barbato <lu_zero@gentoo.org> wrote:
> On 07/04/2017 16:09, James Almer wrote:
>> +int av_spherical_from_name(const char *name);
>
> It can directly output the enum now, it is fine both ways to me.

Wait a minute, I don't think PROJECTION_UNKNOWN makes much sense to
me, given the API and the spec it's based on, and -1 seems not really
informative anyway. In my opinion it would be enough to return int and
just AVERROR(EINVAL) in case it's not found. James would you be ok
with that, or do you have any preference?
Luca Barbato April 10, 2017, 2:08 p.m. | #3
On 10/04/2017 15:56, Vittorio Giovara wrote:
> On Fri, Apr 7, 2017 at 10:22 AM, Luca Barbato <lu_zero@gentoo.org> wrote:
>> On 07/04/2017 16:09, James Almer wrote:
>>> +int av_spherical_from_name(const char *name);
>>
>> It can directly output the enum now, it is fine both ways to me.
> 
> Wait a minute, I don't think PROJECTION_UNKNOWN makes much sense to
> me, given the API and the spec it's based on, and -1 seems not really
> informative anyway. In my opinion it would be enough to return int and
> just AVERROR(EINVAL) in case it's not found. James would you be ok
> with that, or do you have any preference?

I like that.

lu
James Almer April 10, 2017, 3:49 p.m. | #4
On 4/10/2017 10:56 AM, Vittorio Giovara wrote:
> On Fri, Apr 7, 2017 at 10:22 AM, Luca Barbato <lu_zero@gentoo.org> wrote:
>> On 07/04/2017 16:09, James Almer wrote:
>>> +int av_spherical_from_name(const char *name);
>>
>> It can directly output the enum now, it is fine both ways to me.
> 
> Wait a minute, I don't think PROJECTION_UNKNOWN makes much sense to
> me, given the API and the spec it's based on, and -1 seems not really
> informative anyway.

The doxy in the original patch states -1 means not found. It's also
what the same function in stereo3d returns.

> In my opinion it would be enough to return int and
> just AVERROR(EINVAL) in case it's not found. James would you be ok
> with that, or do you have any preference?

IMO, PROJECTION_UNKNOWN is an interesting addition.

Assume for example a projection we don't currently support shows up in
a file in the future, both the mov and matroska demuxers would ignore
it and report the file as having no spherical metadata whatsoever.
With PROJECTION_UNKNOWN the demuxers could report the presence of said
metadata (and the generic values yaw/pitch/roll) while clearly stating
it's unknown.
Muxers would of course keep behaving the same and just not mux unknown
spherical metadata.

But in any case, either -1 or EINVAL are ok with me if you don't like
the above.
Vittorio Giovara April 10, 2017, 4:20 p.m. | #5
On Mon, Apr 10, 2017 at 11:49 AM, James Almer <jamrial@gmail.com> wrote:
>> In my opinion it would be enough to return int and
>> just AVERROR(EINVAL) in case it's not found. James would you be ok
>> with that, or do you have any preference?
>
> IMO, PROJECTION_UNKNOWN is an interesting addition.
>
> Assume for example a projection we don't currently support shows up in
> a file in the future, both the mov and matroska demuxers would ignore
> it and report the file as having no spherical metadata whatsoever.
> With PROJECTION_UNKNOWN the demuxers could report the presence of said
> metadata (and the generic values yaw/pitch/roll) while clearly stating
> it's unknown.

I mean, if it's unknown, no demuxer can parse correctly any field:
nothing guarantees that future projections will keep y/p/r in the same
place. Even so, what's the user going to do with knowing the fact that
the video contains spherical metadata it can't parse correctly/fully
support? So, I'm hesitant to add a new type for this uncertain
behaviour, wouldn't you agree?
James Almer April 10, 2017, 5:43 p.m. | #6
On 4/10/2017 1:20 PM, Vittorio Giovara wrote:
> On Mon, Apr 10, 2017 at 11:49 AM, James Almer <jamrial@gmail.com> wrote:
>>> In my opinion it would be enough to return int and
>>> just AVERROR(EINVAL) in case it's not found. James would you be ok
>>> with that, or do you have any preference?
>>
>> IMO, PROJECTION_UNKNOWN is an interesting addition.
>>
>> Assume for example a projection we don't currently support shows up in
>> a file in the future, both the mov and matroska demuxers would ignore
>> it and report the file as having no spherical metadata whatsoever.
>> With PROJECTION_UNKNOWN the demuxers could report the presence of said
>> metadata (and the generic values yaw/pitch/roll) while clearly stating
>> it's unknown.
> 
> I mean, if it's unknown, no demuxer can parse correctly any field:
> nothing guarantees that future projections will keep y/p/r in the same
> place.

y/p/r are part of the prhd box in ISOBMFF. If that were to change,
the version field would be updated (We're currently skipping the
version field, so maybe we should add a check right now).
Projection specific boxes are currently cbmp, equi and mesh, which
makes me realize that we don't even need to think of hypothetical
future projections. For mesh files the unknown projection enum would
come in handy today.

In Matroska, y/p/r are standalone elements that may or may not be
present somewhere inside the Spherical master.

> Even so, what's the user going to do with knowing the fact that
> the video contains spherical metadata it can't parse correctly/fully
> support? So, I'm hesitant to add a new type for this uncertain
> behaviour, wouldn't you agree?

No, because that's the whole point of avprobe, the dump.c code, and
other tools using the libraries: Informing the user of the contents
and characteristics of their files.
avconv and similar tools are what attempt to do something with them,
and for unknown projections they would do nothing.

libavformat as it is right now will be aware that there's spherical
information in such files, but purposely pretend it's not there.
I personally think that's not the best behavior.
Vittorio Giovara April 10, 2017, 8:31 p.m. | #7
On Mon, Apr 10, 2017 at 1:43 PM, James Almer <jamrial@gmail.com> wrote:
>> Even so, what's the user going to do with knowing the fact that
>> the video contains spherical metadata it can't parse correctly/fully
>> support? So, I'm hesitant to add a new type for this uncertain
>> behaviour, wouldn't you agree?
>
> No, because that's the whole point of avprobe, the dump.c code, and
> other tools using the libraries: Informing the user of the contents
> and characteristics of their files.
> avconv and similar tools are what attempt to do something with them,
> and for unknown projections they would do nothing.

I don't know, I feel like we're discussing about what kind of noise a
tree would do falling in an empty forest :)
I guess we should wait for other people to chime in, and solve this knot
Luca Barbato April 17, 2017, 11:03 p.m. | #8
On 10/04/2017 22:31, Vittorio Giovara wrote:
> On Mon, Apr 10, 2017 at 1:43 PM, James Almer <jamrial@gmail.com> wrote:
>>> Even so, what's the user going to do with knowing the fact that
>>> the video contains spherical metadata it can't parse correctly/fully
>>> support? So, I'm hesitant to add a new type for this uncertain
>>> behaviour, wouldn't you agree?
>>
>> No, because that's the whole point of avprobe, the dump.c code, and
>> other tools using the libraries: Informing the user of the contents
>> and characteristics of their files.
>> avconv and similar tools are what attempt to do something with them,
>> and for unknown projections they would do nothing.
> 
> I don't know, I feel like we're discussing about what kind of noise a
> tree would do falling in an empty forest :)
> I guess we should wait for other people to chime in, and solve this knot
> 

I'm happy as long the function does return an AVERROR on failure or
something easy to match, if I recall correctly Anton seems to be more
for not reporting possibly misleading data when it got discussed on IRC.
(Anton please confirm :))

I'd go with your initial patch amended to return AVERROR(ENOSYS) at
least for now.

lu

Patch

diff --git a/doc/APIchanges b/doc/APIchanges
index a0ca3b7ac..bafac8b4a 100644
--- a/doc/APIchanges
+++ b/doc/APIchanges
@@ -13,6 +13,10 @@  libavutil:     2017-03-23
 
 API changes, most recent first:
 
+2017-04-xx - xxxxxxx - lavu 56.1.0 - spherical.h
+  Add AV_SPHERICAL_UNKNOWN, av_spherical_projection_name(),
+  av_spherical_from_name().
+
 2017-03-xx - xxxxxxx - lavc 57.37.0 - avcodec.h
   Add AVCodecContext.hwaccel_flags field. This will control some hwaccels at
   a later point.
diff --git a/libavutil/spherical.c b/libavutil/spherical.c
index f5accc487..127d7e5b8 100644
--- a/libavutil/spherical.c
+++ b/libavutil/spherical.c
@@ -18,6 +18,7 @@ 
  * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
  */
 
+#include "common.h"
 #include "mem.h"
 #include "spherical.h"
 
@@ -50,3 +51,30 @@  void av_spherical_tile_bounds(AVSphericalMapping *map,
     *right  = orig_width  - width  - *left;
     *bottom = orig_height - height - *top;
 }
+
+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)
+{
+    if ((unsigned)projection >= FF_ARRAY_ELEMS(spherical_projection_names))
+        return "unknown";
+
+    return spherical_projection_names[projection];
+}
+
+int av_spherical_from_name(const char *name)
+{
+    int i;
+
+    for (i = 0; i < FF_ARRAY_ELEMS(spherical_projection_names); i++) {
+        size_t len = strlen(spherical_projection_names[i]);
+        if (!strncmp(spherical_projection_names[i], name, len))
+            return i;
+    }
+
+    return AV_SPHERICAL_UNKNOWN;
+}
diff --git a/libavutil/spherical.h b/libavutil/spherical.h
index fd662cf67..ffd52c69c 100644
--- a/libavutil/spherical.h
+++ b/libavutil/spherical.h
@@ -50,6 +50,11 @@ 
  */
 enum AVSphericalProjection {
     /**
+     * Unknown projection.
+     */
+    AV_SPHERICAL_UNKNOWN = -1,
+
+    /**
      * Video represents a sphere mapped on a flat surface using
      * equirectangular projection.
      */
@@ -206,6 +211,25 @@  void av_spherical_tile_bounds(AVSphericalMapping *map,
                               size_t width, size_t height,
                               size_t *left, size_t *top,
                               size_t *right, size_t *bottom);
+
+/**
+ * Provide a human-readable name of a given AVSphericalProjection.
+ *
+ * @param projection The input AVSphericalProjection.
+ *
+ * @return The name of the AVSphericalProjection, or "unknown".
+ */
+const char *av_spherical_projection_name(enum AVSphericalProjection projection);
+
+/**
+ * Get the AVSphericalProjection form a human-readable name.
+ *
+ * @param name The input string.
+ *
+ * @return The AVSphericalProjection value if found, AV_SPHERICAL_UNKNOWN
+ *         otherwise.
+ */
+int av_spherical_from_name(const char *name);
 /**
  * @}
  * @}
diff --git a/libavutil/version.h b/libavutil/version.h
index b8425ea2c..fd72ff431 100644
--- a/libavutil/version.h
+++ b/libavutil/version.h
@@ -54,7 +54,7 @@ 
  */
 
 #define LIBAVUTIL_VERSION_MAJOR 56
-#define LIBAVUTIL_VERSION_MINOR  0
+#define LIBAVUTIL_VERSION_MINOR  1
 #define LIBAVUTIL_VERSION_MICRO  0
 
 #define LIBAVUTIL_VERSION_INT   AV_VERSION_INT(LIBAVUTIL_VERSION_MAJOR, \