diff mbox

[FFmpeg-devel] mov, matroskadec : Allow matroskadec & mov to share spherical parsing logic.

Message ID CAA0c1bCMjdKOzX2qgyxzxgj1q211GzJ6x2wfSvoAgPaNteduhA@mail.gmail.com
State New
Headers show

Commit Message

Aaron Colwell Feb. 6, 2017, 9:59 p.m. UTC
- Extracts common spherical metadata parsing logic.
- Adds checks to enforce that only non-tiled equirect & non-padded cubemaps
  are accepted.

Comments

Vittorio Giovara Feb. 8, 2017, 1:47 p.m. UTC | #1
On Mon, Feb 6, 2017 at 4:59 PM, Aaron Colwell <acolwell@google.com> wrote:
> - Extracts common spherical metadata parsing logic.
> - Adds checks to enforce that only non-tiled equirect & non-padded cubemaps
>   are accepted.

Hi Aaron,
this patch basically ignores all my comments, so I'm not very happy with it.

I believe the biggest complaint of mine is that this implementation
only works with the ffmpeg command line and leaves users in the dark,
in addition to everything I said in the other thread. There are other
cosmetics oddities (such as adding a *mov* dependency to the
*matroska* code, or mixing code move with new functionality) which are
practices that should be avoided.

If you don't mind, I'll try to work on something that should finally
settle this and provide a patch.
Cheers
James Almer Feb. 8, 2017, 2:35 p.m. UTC | #2
On 2/8/2017 10:47 AM, Vittorio Giovara wrote:
> On Mon, Feb 6, 2017 at 4:59 PM, Aaron Colwell <acolwell@google.com> wrote:
>> - Extracts common spherical metadata parsing logic.
>> - Adds checks to enforce that only non-tiled equirect & non-padded cubemaps
>>   are accepted.
> 
> Hi Aaron,
> this patch basically ignores all my comments, so I'm not very happy with it.
> 
> I believe the biggest complaint of mine is that this implementation
> only works with the ffmpeg command line and leaves users in the dark,
> in addition to everything I said in the other thread. There are other
> cosmetics oddities (such as adding a *mov* dependency to the
> *matroska* code

The matroska elements contain the spherical data in the same structure
as they are in mov files. Much like we're including oggdec.h or flac.h
to parse vorbis comment and flac headers, including a new header to
parse an specific kind of mov atom available in other formats should be
ok.

After all, the alternative would be duplicate the parsing code in two
demuxers.

, or mixing code move with new functionality) which are
> practices that should be avoided.
> 
> If you don't mind, I'll try to work on something that should finally
> settle this and provide a patch.
> Cheers
>
Vittorio Giovara Feb. 8, 2017, 2:41 p.m. UTC | #3
On Wed, Feb 8, 2017 at 9:35 AM, James Almer <jamrial@gmail.com> wrote:
> On 2/8/2017 10:47 AM, Vittorio Giovara wrote:
>> On Mon, Feb 6, 2017 at 4:59 PM, Aaron Colwell <acolwell@google.com> wrote:
>>> - Extracts common spherical metadata parsing logic.
>>> - Adds checks to enforce that only non-tiled equirect & non-padded cubemaps
>>>   are accepted.
>>
>> Hi Aaron,
>> this patch basically ignores all my comments, so I'm not very happy with it.
>>
>> I believe the biggest complaint of mine is that this implementation
>> only works with the ffmpeg command line and leaves users in the dark,
>> in addition to everything I said in the other thread. There are other
>> cosmetics oddities (such as adding a *mov* dependency to the
>> *matroska* code
>
> The matroska elements contain the spherical data in the same structure
> as they are in mov files. Much like we're including oggdec.h or flac.h
> to parse vorbis comment and flac headers, including a new header to
> parse an specific kind of mov atom available in other formats should be
> ok.
>
> After all, the alternative would be duplicate the parsing code in two
> demuxers.

..which is what we do for most of the other places (replaygain,
stereo3d, mastering display and so on and on)..
Besides, that was just a cosmetic comment, if the code is to be shared
among formats (which I believe is totally unnecessary for _just two_
demuxers) dropping the 'mov' prefix from the file would do, in my
opinion.
diff mbox

Patch

From 218dfd7ca1924bc44a168b34ec3c07051823c2dc Mon Sep 17 00:00:00 2001
From: Aaron Colwell <acolwell@google.com>
Date: Mon, 6 Feb 2017 13:48:58 -0800
Subject: [PATCH] mov,matroskadec : Allow matroskadec & mov to share spherical
 parsing logic.

- Extracts common spherical metadata parsing logic.
- Adds checks to enforce that only non-tiled equirect & non-padded cubemaps
  are accepted.
---
 libavformat/Makefile        |  5 +--
 libavformat/matroskadec.c   | 39 ++++++++++++-----------
 libavformat/mov.c           | 35 +++++++++++---------
 libavformat/mov_spherical.c | 78 +++++++++++++++++++++++++++++++++++++++++++++
 libavformat/mov_spherical.h | 61 +++++++++++++++++++++++++++++++++++
 5 files changed, 182 insertions(+), 36 deletions(-)
 create mode 100644 libavformat/mov_spherical.c
 create mode 100644 libavformat/mov_spherical.h

diff --git a/libavformat/Makefile b/libavformat/Makefile
index fc2d76067b..15ec4e42b6 100644
--- a/libavformat/Makefile
+++ b/libavformat/Makefile
@@ -265,7 +265,8 @@  OBJS-$(CONFIG_M4V_MUXER)                 += rawenc.o
 OBJS-$(CONFIG_MATROSKA_DEMUXER)          += matroskadec.o matroska.o  \
                                             rmsipr.o flac_picture.o \
                                             oggparsevorbis.o vorbiscomment.o \
-                                            flac_picture.o replaygain.o
+                                            flac_picture.o replaygain.o \
+                                            mov_spherical.o
 OBJS-$(CONFIG_MATROSKA_MUXER)            += matroskaenc.o matroska.o \
                                             avc.o hevc.o \
                                             flacenc_header.o avlanguage.o vorbiscomment.o wv.o \
@@ -283,7 +284,7 @@  OBJS-$(CONFIG_MLV_DEMUXER)               += mlvdec.o riffdec.o
 OBJS-$(CONFIG_MM_DEMUXER)                += mm.o
 OBJS-$(CONFIG_MMF_DEMUXER)               += mmf.o
 OBJS-$(CONFIG_MMF_MUXER)                 += mmf.o rawenc.o
-OBJS-$(CONFIG_MOV_DEMUXER)               += mov.o mov_chan.o replaygain.o
+OBJS-$(CONFIG_MOV_DEMUXER)               += mov.o mov_chan.o mov_spherical.o replaygain.o
 OBJS-$(CONFIG_MOV_MUXER)                 += movenc.o avc.o hevc.o vpcc.o \
                                             movenchint.o mov_chan.o rtp.o \
                                             movenccenc.o rawutils.o
diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
index 7223e94b55..ea747e76b2 100644
--- a/libavformat/matroskadec.c
+++ b/libavformat/matroskadec.c
@@ -54,6 +54,7 @@ 
 #include "internal.h"
 #include "isom.h"
 #include "matroska.h"
+#include "mov_spherical.h"
 #include "oggdec.h"
 /* For ff_codec_get_id(). */
 #include "riff.h"
@@ -1909,36 +1910,36 @@  static int mkv_parse_video_color(AVStream *st, const MatroskaTrack *track) {
     return 0;
 }
 
-static int mkv_parse_video_projection(AVStream *st, const MatroskaTrack *track) {
+static int mkv_parse_video_projection(AVFormatContext *fc, AVStream *st, const MatroskaTrack *track) {
     AVSphericalMapping *spherical;
-    enum AVSphericalProjection projection;
     size_t spherical_size;
-    int ret;
+    int ret = 0;
+    AVIOContext pb;
+
+    spherical = av_spherical_alloc(&spherical_size);
+    if (!spherical)
+        return AVERROR(ENOMEM);
+
+    spherical->yaw   = (int32_t)(track->video.projection.yaw   * (1 << 16));
+    spherical->pitch = (int32_t)(track->video.projection.pitch * (1 << 16));
+    spherical->roll  = (int32_t)(track->video.projection.roll  * (1 << 16));
 
+    ffio_init_context(&pb, track->video.projection.private.data, track->video.projection.private.size,
+                      0, NULL, NULL, NULL, NULL);
     switch (track->video.projection.type) {
     case MATROSKA_VIDEO_PROJECTION_TYPE_EQUIRECTANGULAR:
-        projection = AV_SPHERICAL_EQUIRECTANGULAR;
+        ret = ff_mov_read_equi(fc, &pb, track->video.projection.private.size, spherical);
         break;
     case MATROSKA_VIDEO_PROJECTION_TYPE_CUBEMAP:
-        if (track->video.projection.private.size < 4)
-            return AVERROR_INVALIDDATA;
-        projection = AV_SPHERICAL_CUBEMAP;
+        ret = ff_mov_read_cbmp(fc, &pb, track->video.projection.private.size, spherical);
         break;
     default:
-        return 0;
+        ret = AVERROR_PATCHWELCOME;
     }
 
-    spherical = av_spherical_alloc(&spherical_size);
-    if (!spherical)
-        return AVERROR(ENOMEM);
-    spherical->projection = projection;
-
-    spherical->yaw   = (int32_t)(track->video.projection.yaw   * (1 << 16));
-    spherical->pitch = (int32_t)(track->video.projection.pitch * (1 << 16));
-    spherical->roll  = (int32_t)(track->video.projection.roll  * (1 << 16));
+    if (!ret)
+      ret = av_stream_add_side_data(st, AV_PKT_DATA_SPHERICAL, (uint8_t *)spherical, spherical_size);
 
-    ret = av_stream_add_side_data(st, AV_PKT_DATA_SPHERICAL, (uint8_t *)spherical,
-                                  spherical_size);
     if (ret < 0) {
         av_freep(&spherical);
         return ret;
@@ -2427,7 +2428,7 @@  static int matroska_parse_tracks(AVFormatContext *s)
             ret = mkv_parse_video_color(st, track);
             if (ret < 0)
                 return ret;
-            ret = mkv_parse_video_projection(st, track);
+            ret = mkv_parse_video_projection(matroska->ctx, st, track);
             if (ret < 0)
                 return ret;
         } else if (track->type == MATROSKA_TRACK_TYPE_AUDIO) {
diff --git a/libavformat/mov.c b/libavformat/mov.c
index 6fd43a0a4e..9fdaca0a28 100644
--- a/libavformat/mov.c
+++ b/libavformat/mov.c
@@ -56,6 +56,7 @@ 
 #include "libavcodec/get_bits.h"
 #include "id3v1.h"
 #include "mov_chan.h"
+#include "mov_spherical.h"
 #include "replaygain.h"
 
 #if CONFIG_ZLIB
@@ -4624,7 +4625,7 @@  static int mov_read_sv3d(MOVContext *c, AVIOContext *pb, MOVAtom atom)
     int size;
     int32_t yaw, pitch, roll;
     uint32_t tag;
-    enum AVSphericalProjection projection;
+    int ret = 0;
 
     if (c->fc->nb_streams < 1)
         return 0;
@@ -4676,32 +4677,36 @@  static int mov_read_sv3d(MOVContext *c, AVIOContext *pb, MOVAtom atom)
     roll  = avio_rb32(pb);
 
     size = avio_rb32(pb);
-    if (size > atom.size)
+    if (size < 8 && size > atom.size)
         return AVERROR_INVALIDDATA;
 
     tag = avio_rl32(pb);
-    avio_skip(pb, 4); /*  version + flags */
+
+    sc->spherical = av_spherical_alloc(&sc->spherical_size);
+    if (!sc->spherical)
+        return AVERROR(ENOMEM);
+
+    sc->spherical->yaw   = yaw;
+    sc->spherical->pitch = pitch;
+    sc->spherical->roll  = roll;
+
     switch (tag) {
     case MKTAG('c','b','m','p'):
-        projection = AV_SPHERICAL_CUBEMAP;
+        ret = ff_mov_read_cbmp(c->fc, pb, size - 8, sc->spherical);
         break;
     case MKTAG('e','q','u','i'):
-        projection = AV_SPHERICAL_EQUIRECTANGULAR;
+        ret = ff_mov_read_equi(c->fc, pb, size - 8, sc->spherical);
         break;
     default:
         av_log(c->fc, AV_LOG_ERROR, "Unknown projection type\n");
-        return 0;
+        ret = AVERROR_PATCHWELCOME;
     }
 
-    sc->spherical = av_spherical_alloc(&sc->spherical_size);
-    if (!sc->spherical)
-        return AVERROR(ENOMEM);
-
-    sc->spherical->projection = projection;
-
-    sc->spherical->yaw   = yaw;
-    sc->spherical->pitch = pitch;
-    sc->spherical->roll  = roll;
+    if (ret < 0) {
+      av_freep(&sc->spherical);
+      sc->spherical_size = 0;
+      return ret;
+    }
 
     return 0;
 }
diff --git a/libavformat/mov_spherical.c b/libavformat/mov_spherical.c
new file mode 100644
index 0000000000..a9971142bd
--- /dev/null
+++ b/libavformat/mov_spherical.c
@@ -0,0 +1,78 @@ 
+/*
+ * Copyright (c) 2017 Aaron Colwell
+ *
+ * This file is part of FFmpeg.
+ *
+ * FFmpeg is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * FFmpeg is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with FFmpeg; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
+ */
+
+#include "mov_spherical.h"
+
+int ff_mov_read_equi(AVFormatContext *fc, AVIOContext *pb, int64_t size,
+                     AVSphericalMapping* spherical_mapping)
+{
+    uint32_t projection_bounds_top;
+    uint32_t projection_bounds_bottom;
+    uint32_t projection_bounds_left;
+    uint32_t projection_bounds_right;
+
+    if (size == 0) {
+        spherical_mapping->projection = AV_SPHERICAL_EQUIRECTANGULAR;
+        return 0;
+    }
+
+    if (size < 20)
+        return AVERROR_INVALIDDATA;
+
+    avio_skip(pb, 4); /*  version + flags */
+    projection_bounds_top = avio_rb32(pb);
+    projection_bounds_bottom = avio_rb32(pb);
+    projection_bounds_left = avio_rb32(pb);
+    projection_bounds_right = avio_rb32(pb);
+
+    if (projection_bounds_top != 0 || projection_bounds_bottom != 0 ||
+        projection_bounds_left != 0 || projection_bounds_right != 0) {
+        av_log(fc, AV_LOG_ERROR, "Unsupported equirect bounds. top %u bottom %u left %u right %u\n",
+               projection_bounds_top, projection_bounds_bottom,
+               projection_bounds_left, projection_bounds_right);
+        return AVERROR_PATCHWELCOME;
+    }
+
+    spherical_mapping->projection = AV_SPHERICAL_EQUIRECTANGULAR;
+    return 0;
+}
+
+int ff_mov_read_cbmp(AVFormatContext *fc, AVIOContext *pb, int64_t size,
+                     AVSphericalMapping* spherical_mapping)
+{
+    uint32_t layout;
+    uint32_t padding;
+
+    if (size < 12)
+        return AVERROR_INVALIDDATA;
+
+    avio_skip(pb, 4); /*  version + flags */
+    layout = avio_rb32(pb);
+    padding = avio_rb32(pb);
+
+    if (layout != 0 || padding != 0) {
+        av_log(fc, AV_LOG_ERROR, "Unsupported cubemap parameters. layout %u, padding %u\n",
+               layout, padding);
+        return AVERROR_PATCHWELCOME;
+    }
+
+    spherical_mapping->projection = AV_SPHERICAL_CUBEMAP;
+    return 0;
+}
diff --git a/libavformat/mov_spherical.h b/libavformat/mov_spherical.h
new file mode 100644
index 0000000000..fe514a4b6c
--- /dev/null
+++ b/libavformat/mov_spherical.h
@@ -0,0 +1,61 @@ 
+/*
+ * Copyright (c) 2017 Aaron Colwell
+ *
+ * This file is part of FFmpeg.
+ *
+ * FFmpeg is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * FFmpeg is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with FFmpeg; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
+ */
+
+/**
+ * mov 'equi', 'cbmp' tag reading/writing.
+ * @author Aaron Colwell
+ */
+
+#ifndef AVFORMAT_MOV_SPHERICAL_H
+#define AVFORMAT_MOV_SPHERICAL_H
+
+#include <stdint.h>
+#include "libavcodec/avcodec.h"
+#include "libavformat/avformat.h"
+#include "libavformat/avio.h"
+#include "libavutil/spherical.h"
+
+/**
+ * Parses the contents of an 'equi' box and sets the appropriate fields in spherical_mapping.
+ *
+ * @param fc    AVFormatContext
+ * @param pb    AVIOContext containing 'equi' box data.
+ * @param size  Remaining size in the 'equi' box
+ * @param spherical_mapping Structure updated with parsed data.
+ * @return      0 if ok, or negative AVERROR code on failure
+ *
+ */
+int ff_mov_read_equi(AVFormatContext *fc, AVIOContext *pb, int64_t size,
+                     AVSphericalMapping* spherical_mapping);
+
+/**
+ * Parses the contents of an 'cbmp' box and sets the appropriate fields in spherical_mapping.
+ *
+ * @param fc    AVFormatContext
+ * @param pb    AVIOContext containing 'cbmp' box data.
+ * @param size  Remaining size in the 'cbmp' box
+ * @param spherical_mapping Structure updated with parsed data.
+ * @return      0 if ok, or negative AVERROR code on failure
+ *
+ */
+int ff_mov_read_cbmp(AVFormatContext *fc, AVIOContext *pb, int64_t size,
+                     AVSphericalMapping* spherical_mapping);
+
+#endif /* AVFORMAT_MOV_SPHERICAL_H */
-- 
2.11.0.483.g087da7b7c-goog