Re: [FFmpeg-devel] [PATCH] ffmpeg: Add spherical_mapping command-line option.

[Aaron Colwell]

Hi Mark,

You aren't missing anything. I haven't really used bsf's so it didn't occur
to me to use them. I'll take a look. Thanks for the suggestion.

Aaron

On Fri, Oct 6, 2017 at 8:53 AM Mark Thompson <s...@jkqxz.net> wrote:

> On 06/10/17 16:19, Aaron Colwell wrote:
> > Allows spherical mapping metadata to be injected into files.
> >
> > From 6a86e9766708b9b74e4ae0ec6928a81df4041afc Mon Sep 17 00:00:00 2001
> > From: Aaron Colwell <acolw...@google.com>
> > Date: Fri, 6 Oct 2017 08:14:15 -0700
> > Subject: [PATCH] ffmpeg: Add spherical_mapping command-line option.
> >
> > Allows spherical mapping metadata to be injected into files.
> > ---
> >  fftools/ffmpeg.c |  12 +++-
> >  fftools/ffmpeg.h |   6 ++
> >  fftools/ffmpeg_opt.c | 160
> ++-
> >  3 files changed, 176 insertions(+), 2 deletions(-)
>
> Perhaps I'm missing something about this patch, but I'm not seeing why
> this should be added as a set of special options to ffmpeg.c.  A bsf in
> libavcodec which edits or replaces the packet side data would seem to be a
> more generally useful way to achieve the same result.  (E.g. it would also
> be usable in other applications, and it would be able to edit it on input
> as well as output.)
>
> - Mark
> ___
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCH] ffmpeg: Add spherical_mapping command-line option.

[Aaron Colwell]

Allows spherical mapping metadata to be injected into files.
From 6a86e9766708b9b74e4ae0ec6928a81df4041afc Mon Sep 17 00:00:00 2001
From: Aaron Colwell <acolw...@google.com>
Date: Fri, 6 Oct 2017 08:14:15 -0700
Subject: [PATCH] ffmpeg: Add spherical_mapping command-line option.

Allows spherical mapping metadata to be injected into files.
---
 fftools/ffmpeg.c |  12 +++-
 fftools/ffmpeg.h |   6 ++
 fftools/ffmpeg_opt.c | 160 ++-
 3 files changed, 176 insertions(+), 2 deletions(-)

diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c
index 1d248bc269..8c35090b9e 100644
--- a/fftools/ffmpeg.c
+++ b/fftools/ffmpeg.c
@@ -3092,6 +3092,10 @@ static int init_output_stream_streamcopy(OutputStream *ost)
 const AVPacketSideData *sd_src = >st->side_data[i];
 uint8_t *dst_data;
 
+if ((sd_src->type == AV_PKT_DATA_SPHERICAL && ost->spherical_mapping_overridden) ||
+(sd_src->type == AV_PKT_DATA_STEREO3D && ost->stereo3d_overridden))
+continue;
+
 dst_data = av_stream_new_side_data(ost->st, sd_src->type, sd_src->size);
 if (!dst_data)
 return AVERROR(ENOMEM);
@@ -3528,7 +3532,13 @@ static int init_output_stream(OutputStream *ost, char *error, int error_len)
 int i;
 for (i = 0; i < ist->st->nb_side_data; i++) {
 AVPacketSideData *sd = >st->side_data[i];
-uint8_t *dst = av_stream_new_side_data(ost->st, sd->type, sd->size);
+uint8_t *dst;
+
+if ((sd->type == AV_PKT_DATA_SPHERICAL && ost->spherical_mapping_overridden) ||
+(sd->type == AV_PKT_DATA_STEREO3D && ost->stereo3d_overridden))
+continue;
+
+dst = av_stream_new_side_data(ost->st, sd->type, sd->size);
 if (!dst)
 return AVERROR(ENOMEM);
 memcpy(dst, sd->data, sd->size);
diff --git a/fftools/ffmpeg.h b/fftools/ffmpeg.h
index f6c76bcc55..91ca6c2926 100644
--- a/fftools/ffmpeg.h
+++ b/fftools/ffmpeg.h
@@ -45,6 +45,8 @@
 #include "libavutil/hwcontext.h"
 #include "libavutil/pixfmt.h"
 #include "libavutil/rational.h"
+#include "libavutil/spherical.h"
+#include "libavutil/stereo3d.h"
 #include "libavutil/threadmessage.h"
 
 #include "libswresample/swresample.h"
@@ -237,6 +239,8 @@ typedef struct OptionsContext {
 intnb_time_bases;
 SpecifierOpt *enc_time_bases;
 intnb_enc_time_bases;
+SpecifierOpt *spherical_mappings;
+intnb_spherical_mappings;
 } OptionsContext;
 
 typedef struct InputFilter {
@@ -487,6 +491,8 @@ typedef struct OutputStream {
 int top_field_first;
 int rotate_overridden;
 double rotate_override_value;
+int spherical_mapping_overridden;
+int stereo3d_overridden;
 
 AVRational frame_aspect_ratio;
 
diff --git a/fftools/ffmpeg_opt.c b/fftools/ffmpeg_opt.c
index 100fa76e46..4bc5104ce5 100644
--- a/fftools/ffmpeg_opt.c
+++ b/fftools/ffmpeg_opt.c
@@ -40,6 +40,8 @@
 #include "libavutil/parseutils.h"
 #include "libavutil/pixdesc.h"
 #include "libavutil/pixfmt.h"
+#include "libavutil/spherical.h"
+#include "libavutil/stereo3d.h"
 
 #define DEFAULT_PASS_LOGFILENAME_PREFIX "ffmpeg2pass"
 
@@ -1585,12 +1587,158 @@ static void check_streamcopy_filters(OptionsContext *o, AVFormatContext *oc,
 }
 }
 
+static int set_spherical_mapping(const char* opt, OutputStream *ost) {
+typedef struct {
+const AVClass* spherical_class;
+int projection;
+
+double yaw;
+double pitch;
+double roll;
+
+int64_t bound_left;
+int64_t bound_top;
+int64_t bound_right;
+int64_t bound_bottom;
+
+int64_t padding;
+
+int stereo_mode;
+} SphericalMappingContext;
+
+#define OFFSET(x) offsetof(SphericalMappingContext, x)
+#define FLAGS AV_OPT_FLAG_VIDEO_PARAM
+
+static const AVOption opts[] = {
+{ "projection", "projection", OFFSET(projection), AV_OPT_TYPE_INT,
+{ .i64 = -1 }, -1, AV_SPHERICAL_EQUIRECTANGULAR_TILE, FLAGS, "projection" },
+{ "equirectangular", "equirectangular projection", 0, AV_OPT_TYPE_CONST,
+{ .i64 = AV_SPHERICAL_EQUIRECTANGULAR }, INT_MIN, INT_MAX, FLAGS, "projection" },
+{ "cubemap", "cubemap projection", 0, AV_OPT_TYPE_CONST,
+{ .i64 = AV_SPHERICAL_CUBEMAP }, INT_MIN, INT_MAX, FLAGS, "projection" },
+{ "equirectangular_tile", "tiled equirectangular projection", 0, AV_OPT_TYPE_CONST,
+{ .i64 = AV_SPHERICAL_EQUIRECTANGULAR_TILE }, 

Re: [FFmpeg-devel] [PATCH] Add spherical_mapping command-line argument to ffmpeg.

[Aaron Colwell]

Attached a new patch that fixes a bug and the indentation in the previous
patch. This should be good now. Sorry for the spam.

Aaron

On Mon, Jun 5, 2017 at 12:32 PM Aaron Colwell <acolw...@google.com> wrote:

> Comments below..
>
> On Mon, Jun 5, 2017 at 12:02 PM Vittorio Giovara <
> vittorio.giov...@gmail.com> wrote:
>
>> Hey Aaron
>> > This allows adding AVSphericalMapping information to files
>> > that don't already have it.
>> > ---
>> >  ffmpeg.h  |   3 ++
>> >  ffmpeg_opt.c  |  29 -
>> >  libavutil/spherical.c | 113
>> ++
>> >  libavutil/spherical.h |  14 +++
>> >  4 files changed, 158 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/ffmpeg.h b/ffmpeg.h
>> > index a806445e0d..43a28d874f 100644
>> > --- a/ffmpeg.h
>> > +++ b/ffmpeg.h
>> > @@ -44,6 +44,7 @@
>> >  #include "libavutil/fifo.h"
>> >  #include "libavutil/pixfmt.h"
>> >  #include "libavutil/rational.h"
>> > +#include "libavutil/spherical.h"
>> >  #include "libavutil/threadmessage.h"
>> >
>> >  #include "libswresample/swresample.h"
>> > @@ -228,6 +229,8 @@ typedef struct OptionsContext {
>> >  intnb_time_bases;
>> >  SpecifierOpt *enc_time_bases;
>> >  intnb_enc_time_bases;
>> > +SpecifierOpt *spherical_mappings;
>> > +intnb_spherical_mappings;
>> >  } OptionsContext;
>> >
>> >  typedef struct InputFilter {
>> > diff --git a/ffmpeg_opt.c b/ffmpeg_opt.c
>> > index c997ea8faf..666b3791b7 100644
>> > --- a/ffmpeg_opt.c
>> > +++ b/ffmpeg_opt.c
>> > @@ -1514,12 +1514,29 @@ static void
>> check_streamcopy_filters(OptionsContext *o, AVFormatContext *oc,
>> >  }
>> >  }
>> >
>> > +static int set_spherical_mapping(const char* opt, AVStream* st) {
>> > +  size_t spherical_mapping_size = 0;
>> > +  AVSphericalMapping *spherical_mapping = NULL;
>> > +
>> > +  int ret = av_spherical_parse_option(opt, _mapping,
>> > +  _mapping_size);
>> > +  if (ret >= 0) {
>> > +ret = av_stream_add_side_data(st, AV_PKT_DATA_SPHERICAL,
>> spherical_mapping, spherical_mapping_size);
>> > +
>> > +if (ret < 0) {
>> > +  av_freep(_mapping);
>> > +}
>> > +  }
>> > +
>> > +  return ret;
>> > +}
>> > +
>> >  static OutputStream *new_video_stream(OptionsContext *o,
>> AVFormatContext *oc, int source_index)
>> >  {
>> >  AVStream *st;
>> >  OutputStream *ost;
>> >  AVCodecContext *video_enc;
>> > -char *frame_rate = NULL, *frame_aspect_ratio = NULL;
>> > +char *frame_rate = NULL, *frame_aspect_ratio = NULL,
>> *spherical_mapping = NULL;
>> >
>> >  ost = new_output_stream(o, oc, AVMEDIA_TYPE_VIDEO, source_index);
>> >  st  = ost->st;
>> > @@ -1546,6 +1563,12 @@ static OutputStream
>> *new_video_stream(OptionsContext *o, AVFormatContext *oc, in
>> >
>> >  MATCH_PER_STREAM_OPT(filter_scripts, str, ost->filters_script, oc,
>> st);
>> >  MATCH_PER_STREAM_OPT(filters,str, ost->filters,oc,
>> st);
>> > +MATCH_PER_STREAM_OPT(spherical_mappings, str, spherical_mapping,
>> oc, st);
>> > +
>> > +if (spherical_mapping && set_spherical_mapping(spherical_mapping,
>> st) < 0) {
>> > +av_log(NULL, AV_LOG_FATAL, "Invalid spherical_mapping: %s\n",
>> spherical_mapping);
>> > +exit_program(1);
>> > +}
>> >
>> >  if (!ost->stream_copy) {
>> >  const char *p = NULL;
>> > @@ -3569,6 +3592,10 @@ const OptionDef options[] = {
>> >  "automatically insert correct rotate filters" },
>> >  { "hwaccel_lax_profile_check", OPT_BOOL | OPT_EXPERT,
>>   { _lax_profile_check},
>> >  "attempt to decode anyway if HW accelerated decoder's
>> supported profiles do not exactly match the stream" },
>> > +{ "spherical_mapping", OPT_VIDEO | HAS_ARG  | OPT_STRING |
>> OPT_SPEC |
>> > +   OPT_OUTPUT,
>>{ .off = OFFSET(spherical_mappings) },
>> > +"set spherica

Re: [FFmpeg-devel] [PATCH] Add spherical_mapping command-line argument to ffmpeg.

[Aaron Colwell]

> 0 || ctx.bound_right > 0
> ||
> > + ctx.bound_bottom > 0) && ctx.projection !=
> AV_SPHERICAL_EQUIRECTANGULAR_TILE) {
> > +av_log(NULL, AV_LOG_ERROR, "bounds only allowed for
> AV_SPHERICAL_EQUIRECTANGULAR_TILE projection.\n");
> > +return AVERROR(EINVAL);
> > +}
> > +
> > +*spherical_mapping = av_spherical_alloc(spherical_mapping_size);
> > +if (!*spherical_mapping)
> > +return AVERROR(ENOMEM);
> > +
> > +(*spherical_mapping)->projection = (enum
> AVSphericalProjection)ctx.projection;
> > +(*spherical_mapping)->yaw = (int32_t)(ctx.yaw * (1 << 16));
> > +(*spherical_mapping)->pitch = (int32_t)(ctx.pitch * (1 << 16));
> > +(*spherical_mapping)->roll = (int32_t)(ctx.roll * (1 << 16));
> > +
> > +if (ctx.projection == AV_SPHERICAL_CUBEMAP) {
> > +(*spherical_mapping)->padding = (uint32_t)ctx.padding;
> > +} else if (ctx.projection == AV_SPHERICAL_EQUIRECTANGULAR_TILE) {
> > +(*spherical_mapping)->bound_left = (uint32_t)ctx.bound_left;
> > +(*spherical_mapping)->bound_top = (uint32_t)ctx.bound_top;
> > +(*spherical_mapping)->bound_right = (uint32_t)ctx.bound_right;
> > +(*spherical_mapping)->bound_bottom = (uint32_t)ctx.bound_bottom;
> > +}
> > +
> > +return ret;
> > +}
> > diff --git a/libavutil/spherical.h b/libavutil/spherical.h
> > index cef759cf27..643cff7f44 100644
> > --- a/libavutil/spherical.h
> > +++ b/libavutil/spherical.h
> > @@ -190,6 +190,20 @@ typedef struct AVSphericalMapping {
> >   */
> >  AVSphericalMapping *av_spherical_alloc(size_t *size);
> >
> > +/**
> > + * Parses a command-line option into an AVSphericalMapping object.
> > + *
> > + * @param opts String containing comma separated list of key=value
> pairs.
> > + * @param spherical_mapping Output parameter for the AVSphericalMapping
> created by this function.
> > + * @param spherical_mapping_size Output parameter for indicating the
> size of the struct
> > + * referenced by *spherical_mapping.
> > + *
> > + * @return 0 on success and *spherical_mapping and
> *spherical_mapping_size contain valid information.
> > + *<0 on failure and *spherical_mapping and
> *spherical_mapping_size state is undefined.
> > + */
> > +int av_spherical_parse_option(const char* opt, AVSphericalMapping
> **spherical_mapping,
> > +  size_t *spherical_mapping_size);
> > +
> >  /**
> >   * Convert the @ref bounding fields from an AVSphericalVideo
> >   * from 0.32 fixed point to pixels.
>
> This is the part that I have a problem with. The code itself is
> totally fine, but I do believe that this API should not be exposed
> publicly.
> It's too custom specific and binds the ffmpeg option syntax to
> libavutil while they are two separate layers that need to stay
> separated.
> It should be either something more generic so that other side data can
> attach to it, or it should have its own proper syntax and AVOption
> type.
> For the task at hand though I believe it would be sufficient to move
> option parsing in ffmpeg_opt.c
>

Fair enough. I was on the fence about this but didn't feel strongly either
way. I've attached a new patch that only contains changes in ffmpeg.h and
ffmpeg_opt.c.

Thanks for the quick review.

Aaron


>
> Cheers
> --
> Vittorio
> ___
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
From fd520ed807ddd843d9848aaf57695af3e0abf97e Mon Sep 17 00:00:00 2001
From: Aaron Colwell <acolw...@google.com>
Date: Fri, 2 Jun 2017 16:11:21 -0700
Subject: [PATCH] Add spherical_mapping command-line argument to ffmpeg.

This allows adding AVSphericalMapping information to files
that don't already have it.
---
 ffmpeg.h |   3 ++
 ffmpeg_opt.c | 131 ++-
 2 files changed, 133 insertions(+), 1 deletion(-)

diff --git a/ffmpeg.h b/ffmpeg.h
index a806445e0d..43a28d874f 100644
--- a/ffmpeg.h
+++ b/ffmpeg.h
@@ -44,6 +44,7 @@
 #include "libavutil/fifo.h"
 #include "libavutil/pixfmt.h"
 #include "libavutil/rational.h"
+#include "libavutil/spherical.h"
 #include "libavutil/threadmessage.h"
 
 #include "libswresample/swresample.h"
@@ -228,6 +229,8 @@ typedef struct OptionsContext {
 intnb_time_bases;
 SpecifierOpt *enc_time_bases;
 intnb_enc_time_bases;
+SpecifierOpt *spherical_mappings;
+intnb_

[FFmpeg-devel] [PATCH] Add spherical_mapping command-line argument to ffmpeg.

[Aaron Colwell]

Add spherical_mapping command-line argument to ffmpeg.

This allows adding AVSphericalMapping information to files
that don't already have it.
From eaa84a5f86d37e7de8bd0d6f72a308af57f3ef1d Mon Sep 17 00:00:00 2001
From: Aaron Colwell <acolw...@google.com>
Date: Fri, 2 Jun 2017 16:11:21 -0700
Subject: [PATCH] Add spherical_mapping command-line argument to ffmpeg.

This allows adding AVSphericalMapping information to files
that don't already have it.
---
 ffmpeg.h  |   3 ++
 ffmpeg_opt.c  |  29 -
 libavutil/spherical.c | 113 ++
 libavutil/spherical.h |  14 +++
 4 files changed, 158 insertions(+), 1 deletion(-)

diff --git a/ffmpeg.h b/ffmpeg.h
index a806445e0d..43a28d874f 100644
--- a/ffmpeg.h
+++ b/ffmpeg.h
@@ -44,6 +44,7 @@
 #include "libavutil/fifo.h"
 #include "libavutil/pixfmt.h"
 #include "libavutil/rational.h"
+#include "libavutil/spherical.h"
 #include "libavutil/threadmessage.h"
 
 #include "libswresample/swresample.h"
@@ -228,6 +229,8 @@ typedef struct OptionsContext {
 intnb_time_bases;
 SpecifierOpt *enc_time_bases;
 intnb_enc_time_bases;
+SpecifierOpt *spherical_mappings;
+intnb_spherical_mappings;
 } OptionsContext;
 
 typedef struct InputFilter {
diff --git a/ffmpeg_opt.c b/ffmpeg_opt.c
index c997ea8faf..666b3791b7 100644
--- a/ffmpeg_opt.c
+++ b/ffmpeg_opt.c
@@ -1514,12 +1514,29 @@ static void check_streamcopy_filters(OptionsContext *o, AVFormatContext *oc,
 }
 }
 
+static int set_spherical_mapping(const char* opt, AVStream* st) {
+  size_t spherical_mapping_size = 0;
+  AVSphericalMapping *spherical_mapping = NULL;
+
+  int ret = av_spherical_parse_option(opt, _mapping,
+  _mapping_size);
+  if (ret >= 0) {
+ret = av_stream_add_side_data(st, AV_PKT_DATA_SPHERICAL, spherical_mapping, spherical_mapping_size);
+
+if (ret < 0) {
+  av_freep(_mapping);
+}
+  }
+
+  return ret;
+}
+
 static OutputStream *new_video_stream(OptionsContext *o, AVFormatContext *oc, int source_index)
 {
 AVStream *st;
 OutputStream *ost;
 AVCodecContext *video_enc;
-char *frame_rate = NULL, *frame_aspect_ratio = NULL;
+char *frame_rate = NULL, *frame_aspect_ratio = NULL, *spherical_mapping = NULL;
 
 ost = new_output_stream(o, oc, AVMEDIA_TYPE_VIDEO, source_index);
 st  = ost->st;
@@ -1546,6 +1563,12 @@ static OutputStream *new_video_stream(OptionsContext *o, AVFormatContext *oc, in
 
 MATCH_PER_STREAM_OPT(filter_scripts, str, ost->filters_script, oc, st);
 MATCH_PER_STREAM_OPT(filters,str, ost->filters,oc, st);
+MATCH_PER_STREAM_OPT(spherical_mappings, str, spherical_mapping, oc, st);
+
+if (spherical_mapping && set_spherical_mapping(spherical_mapping, st) < 0) {
+av_log(NULL, AV_LOG_FATAL, "Invalid spherical_mapping: %s\n", spherical_mapping);
+exit_program(1);
+}
 
 if (!ost->stream_copy) {
 const char *p = NULL;
@@ -3569,6 +3592,10 @@ const OptionDef options[] = {
 "automatically insert correct rotate filters" },
 { "hwaccel_lax_profile_check", OPT_BOOL | OPT_EXPERT,{ _lax_profile_check},
 "attempt to decode anyway if HW accelerated decoder's supported profiles do not exactly match the stream" },
+{ "spherical_mapping", OPT_VIDEO | HAS_ARG  | OPT_STRING | OPT_SPEC |
+   OPT_OUTPUT,   { .off = OFFSET(spherical_mappings) },
+"set spherical mapping for video stream", "spherical_mapping" },
+
 
 /* audio options */
 { "aframes",OPT_AUDIO | HAS_ARG  | OPT_PERFILE | OPT_OUTPUT,   { .func_arg = opt_audio_frames },
diff --git a/libavutil/spherical.c b/libavutil/spherical.c
index 4be55f36cf..508584d61f 100644
--- a/libavutil/spherical.c
+++ b/libavutil/spherical.c
@@ -19,6 +19,7 @@
  */
 
 #include "mem.h"
+#include "opt.h"
 #include "spherical.h"
 
 AVSphericalMapping *av_spherical_alloc(size_t *size)
@@ -77,3 +78,115 @@ int av_spherical_from_name(const char *name)
 
 return -1;
 }
+
+static const char *spherical_mapping_context_to_name(void *ptr)
+{
+return "spherical_mapping";
+}
+
+typedef struct {
+const AVClass* spherical_class;
+int projection;
+
+double yaw;
+double pitch;
+double roll;
+
+int64_t bound_left;
+int64_t bound_top;
+int64_t bound_right;
+int64_t bound_bottom;
+
+int64_t padding;
+} SphericalMappingContext;
+
+#define OFFSET(x) offsetof(SphericalMappingContext, x)
+#define DEFAULT 0
+#define FLAGS AV_OPT_FLAG_VIDEO_PARAM
+
+static const AVOption spherical_mapping_options[] = {
+{ "projection", "projection"

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

[Aaron Colwell]

On Thu, Mar 30, 2017 at 7:17 AM James Almer  wrote:

> On 3/30/2017 5:54 AM, Vittorio Giovara wrote:
> > On Wed, Mar 29, 2017 at 8:01 PM, 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 
> >> ---
> >> 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
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] movenc: Add support for writing st3d and sv3d boxes.

[Aaron Colwell]

Made all suggested changes and attached a new patch. Thanks for the quick
review.

Aaron

On Mon, Mar 27, 2017 at 10:00 AM James Almer <jamr...@gmail.com> wrote:

> On 3/27/2017 1:02 PM, Aaron Colwell wrote:
> >
> > 0001-movenc-Add-support-for-writing-st3d-andsv3d-boxes.patch
> >
> >
> > From 8654212c2f2a3ee404020cf5b948d7db3e6270f2 Mon Sep 17 00:00:00 2001
> > From: Aaron Colwell <acolw...@google.com>
> > Date: Mon, 27 Mar 2017 08:00:12 -0700
> > Subject: [PATCH] movenc: Add support for writing st3d and sv3d boxes.
> >
> > ---
> >  libavformat/movenc.c | 105
> +++
> >  1 file changed, 105 insertions(+)
> >
> > diff --git a/libavformat/movenc.c b/libavformat/movenc.c
> > index 3b4e3b519c..4f408b20fa 100644
> > --- a/libavformat/movenc.c
> > +++ b/libavformat/movenc.c
> > @@ -1603,6 +1603,101 @@ static int mov_write_subtitle_tag(AVIOContext
> *pb, MOVTrack *track)
> >  return update_size(pb, pos);
> >  }
> >
> > +static int mov_write_st3d_tag(AVIOContext *pb, AVStereo3D *stereo_3d)
> > +{
> > +int8_t stereo_mode;
> > +
> > +if (stereo_3d->flags != 0) {
> > +av_log(pb, AV_LOG_WARNING, "Unsupported stereo_3d flags %x.
> st3d not written.\n", stereo_3d->flags);
> > +return 0;
> > +}
> > +
> > +switch (stereo_3d->type) {
> > +case AV_STEREO3D_2D:
> > +stereo_mode = 0;
> > +break;
> > +case AV_STEREO3D_TOPBOTTOM:
> > +stereo_mode = 1;
> > +break;
> > +case AV_STEREO3D_SIDEBYSIDE:
> > +stereo_mode = 2;
> > +break;
> > +default:
> > +av_log(pb, AV_LOG_WARNING, "Unsupported stereo_3d type %d. st3d
> not written.\n", stereo_3d->type);
>
> You could use av_stereo3d_type_name(stereo_3d->type) here.
>

Done.


>
> > +return 0;
> > +}
> > +avio_wb32(pb, 13); /* size */
> > +ffio_wfourcc(pb, "st3d");
> > +avio_wb32(pb, 0); /* version = 0 & flags = 0 */
> > +avio_w8(pb, stereo_mode);
> > +return 13;
> > +}
> > +
> > +static int mov_write_sv3d_tag(AVIOContext *pb, AVSphericalMapping
> *spherical_mapping)
> > +{
> > +int64_t sv3d_pos, svhd_pos, proj_pos;
> > +
> > +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;
> > +}
> > +
> > +sv3d_pos = avio_tell(pb);
> > +avio_wb32(pb, 0);  /* size */
> > +ffio_wfourcc(pb, "sv3d");
> > +
> > +svhd_pos = avio_tell(pb);
> > +avio_wb32(pb, 0);  /* size */
> > +ffio_wfourcc(pb, "svhd");
> > +avio_wb32(pb, 0); /* version = 0 & flags = 0 */
> > +avio_put_str(pb, LIBAVFORMAT_IDENT); /* metadata_source */
>
> You need to use some hardcoded string like "Lavf" or nothing at all if
> AVFMT_FLAG_BITEXACT is set in (AVFormatContext*)->flags.
>

Done.


>
> > +update_size(pb, svhd_pos);
> > +
> > +proj_pos = avio_tell(pb);
> > +avio_wb32(pb, 0); /* size */
> > +ffio_wfourcc(pb, "proj");
> > +
> > +avio_wb32(pb, 24); /* size */
> > +ffio_wfourcc(pb, "prhd");
> > +avio_wb32(pb, 0); /* version = 0 & flags = 0 */
> > +avio_wb32(pb, spherical_mapping->yaw);
> > +avio_wb32(pb, spherical_mapping->pitch);
> > +avio_wb32(pb, spherical_mapping->roll);
> > +
> > +switch (spherical_mapping->projection) {
> > +case AV_SPHERICAL_EQUIRECTANGULAR:
> > +avio_wb32(pb, 28);/* size */
> > +ffio_wfourcc(pb, "equi");
> > +avio_wb32(pb, 0); /* version = 0 & flags = 0 */
> > +avio_wb32(pb, 0); /* projection_bounds_top */
> > +avio_wb32(pb, 0); /* projection_bounds_bottom */
> > +avio_wb32(pb, 0); /* projection_bounds_left */
> > +avio_wb32(pb, 0); /* projection_bounds_right */
> > +break;
> > +case AV_SPHERICAL_EQUIRECTANGULAR_TILE:
> > +avio_wb32(pb, 28);/* size */
> > +ffio_wfourcc(pb, "equi");
> > +avio

[FFmpeg-devel] [PATCH] movenc: Add support for writing st3d and sv3d boxes.

[Aaron Colwell]

From 8654212c2f2a3ee404020cf5b948d7db3e6270f2 Mon Sep 17 00:00:00 2001
From: Aaron Colwell <acolw...@google.com>
Date: Mon, 27 Mar 2017 08:00:12 -0700
Subject: [PATCH] movenc: Add support for writing st3d and sv3d boxes.

---
 libavformat/movenc.c | 105 +++
 1 file changed, 105 insertions(+)

diff --git a/libavformat/movenc.c b/libavformat/movenc.c
index 3b4e3b519c..4f408b20fa 100644
--- a/libavformat/movenc.c
+++ b/libavformat/movenc.c
@@ -1603,6 +1603,101 @@ static int mov_write_subtitle_tag(AVIOContext *pb, MOVTrack *track)
 return update_size(pb, pos);
 }
 
+static int mov_write_st3d_tag(AVIOContext *pb, AVStereo3D *stereo_3d)
+{
+int8_t stereo_mode;
+
+if (stereo_3d->flags != 0) {
+av_log(pb, AV_LOG_WARNING, "Unsupported stereo_3d flags %x. st3d not written.\n", stereo_3d->flags);
+return 0;
+}
+
+switch (stereo_3d->type) {
+case AV_STEREO3D_2D:
+stereo_mode = 0;
+break;
+case AV_STEREO3D_TOPBOTTOM:
+stereo_mode = 1;
+break;
+case AV_STEREO3D_SIDEBYSIDE:
+stereo_mode = 2;
+break;
+default:
+av_log(pb, AV_LOG_WARNING, "Unsupported stereo_3d type %d. st3d not written.\n", stereo_3d->type);
+return 0;
+}
+avio_wb32(pb, 13); /* size */
+ffio_wfourcc(pb, "st3d");
+avio_wb32(pb, 0); /* version = 0 & flags = 0 */
+avio_w8(pb, stereo_mode);
+return 13;
+}
+
+static int mov_write_sv3d_tag(AVIOContext *pb, AVSphericalMapping *spherical_mapping)
+{
+int64_t sv3d_pos, svhd_pos, proj_pos;
+
+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;
+}
+
+sv3d_pos = avio_tell(pb);
+avio_wb32(pb, 0);  /* size */
+ffio_wfourcc(pb, "sv3d");
+
+svhd_pos = avio_tell(pb);
+avio_wb32(pb, 0);  /* size */
+ffio_wfourcc(pb, "svhd");
+avio_wb32(pb, 0); /* version = 0 & flags = 0 */
+avio_put_str(pb, LIBAVFORMAT_IDENT); /* metadata_source */
+update_size(pb, svhd_pos);
+
+proj_pos = avio_tell(pb);
+avio_wb32(pb, 0); /* size */
+ffio_wfourcc(pb, "proj");
+
+avio_wb32(pb, 24); /* size */
+ffio_wfourcc(pb, "prhd");
+avio_wb32(pb, 0); /* version = 0 & flags = 0 */
+avio_wb32(pb, spherical_mapping->yaw);
+avio_wb32(pb, spherical_mapping->pitch);
+avio_wb32(pb, spherical_mapping->roll);
+
+switch (spherical_mapping->projection) {
+case AV_SPHERICAL_EQUIRECTANGULAR:
+avio_wb32(pb, 28);/* size */
+ffio_wfourcc(pb, "equi");
+avio_wb32(pb, 0); /* version = 0 & flags = 0 */
+avio_wb32(pb, 0); /* projection_bounds_top */
+avio_wb32(pb, 0); /* projection_bounds_bottom */
+avio_wb32(pb, 0); /* projection_bounds_left */
+avio_wb32(pb, 0); /* projection_bounds_right */
+break;
+case AV_SPHERICAL_EQUIRECTANGULAR_TILE:
+avio_wb32(pb, 28);/* size */
+ffio_wfourcc(pb, "equi");
+avio_wb32(pb, 0); /* version = 0 & flags = 0 */
+avio_wb32(pb, spherical_mapping->bound_top);
+avio_wb32(pb, spherical_mapping->bound_bottom);
+avio_wb32(pb, spherical_mapping->bound_left);
+avio_wb32(pb, spherical_mapping->bound_right);
+break;
+case AV_SPHERICAL_CUBEMAP:
+avio_wb32(pb, 20);/* size */
+ffio_wfourcc(pb, "cbmp");
+avio_wb32(pb, 0); /* version = 0 & flags = 0 */
+avio_wb32(pb, 0); /* layout */
+avio_wb32(pb, spherical_mapping->padding); /* padding */
+break;
+}
+update_size(pb, proj_pos);
+
+return update_size(pb, sv3d_pos);
+}
+
 static int mov_write_pasp_tag(AVIOContext *pb, MOVTrack *track)
 {
 AVRational sar;
@@ -1873,6 +1968,16 @@ static int mov_write_video_tag(AVIOContext *pb, MOVMuxContext *mov, MOVTrack *tr
 av_log(mov->fc, AV_LOG_WARNING, "Not writing 'colr' atom. Format is not MOV or MP4.\n");
 }
 
+if (mov->fc->strict_std_compliance <= FF_COMPLIANCE_EXPERIMENTAL) {
+  AVStereo3D* stereo_3d = (AVStereo3D*) av_stream_get_side_data(track->st, AV_PKT_DATA_STEREO3D, NULL);
+  AVSphericalMapping* spherical_mapping = (AVSphericalMapping*)av_stream_get_side_data(track->st, AV_PKT_DATA_SPHERICAL, NULL);
+
+  if (stereo_3d)
+  mov_write_st3d_tag(pb, stereo_3d);
+  if (spherical_mapping)
+  mov_write_sv3d_tag(pb, spherical_mapping);
+}
+
 if (track->par->sample_aspect_ratio.den && tr

Re: [FFmpeg-devel] [PATCH 1/3] spherical: Add tiled equirectangular type and projection-specific properties

[Aaron Colwell]

On Thu, Feb 16, 2017 at 11:09 AM Vittorio Giovara <
vittorio.giov...@gmail.com> wrote:

> On Wed, Feb 15, 2017 at 2:54 PM, Aaron Colwell <acolw...@google.com>
> wrote:
> > Hi Vittorio,
> >
> > This may not be a blocker for this patch, but one issue with converting
> the
> > bounds to pixels like you do here is that resizing a video could result
> in
> > incorrect metadata being generated when muxing. If you keep the bounds in
> > the 0.0-1.0 fixed point space this problem doesn't happen since it is a
> > resolution independent representation. If you still want to use pixels
> here
> > then the resizing filter will need to become aware of AVSphericalMapping
> and
> > adjust it accordingly. I think cubemap padding in pixels may have a
> similar
> > issue. This is no way intended to be a blocking comment. It is just
> meant to
> > raise awareness on something you might not have considered.
>
> Hi Aaron,
> this is an interesting point, but leaves up open questions. Do you
> foresee actual cases in which this is going to be a problem? It's not
> the first time we have fixed point fields exported, but it's also true
> that the conversion from a 0.32 system is not very common and could
> leave users puzzled. Regarding padding, is the specification going to
> be updated to fix this potential issue?
> Thank you
> --
> Vittorio
>

Hi Vittorio,

After sleeping on this, I think what you have will be fine. Resizing a
cubemap w/ padding is just going to have to be handled in a special way
because fractional pixel padding isn't going to work right on the GPU. You
really only want to waste a few pixels on padding, not a constant fraction
of the whole frame. Given that we have to handle cubemaps in a special way
for resizing, then my thoughts about resizing equirect aren't really a big
deal. I don't expect any spec changes will be needed for this.

I suppose this just a long way of saying "we will still need a followup for
resizing spherical videos properly, but this patch LGTM."

Thanks again for working on this.
Aaron
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 1/3] spherical: Add tiled equirectangular type and projection-specific properties

[Aaron Colwell]

On Wed, Feb 15, 2017 at 5:52 AM James Almer  wrote:

> On 2/14/2017 11:20 PM, Vittorio Giovara wrote:
> > On Tue, Feb 14, 2017 at 6:54 PM, James Almer  wrote:
> >> On 2/14/2017 5:52 PM, Vittorio Giovara wrote:
> >>> On Fri, Feb 10, 2017 at 6:25 PM, Michael Niedermayer
> >>>  wrote:
>  On Fri, Feb 10, 2017 at 04:11:43PM -0500, Vittorio Giovara wrote:
> > Signed-off-by: Vittorio Giovara 
> > ---
> > This should help not losing details over muxing and allows
> > callers to get additional information in a clean manner.
> >
> > Please keep me in CC.
> > Vittorio
> >
> >  doc/APIchanges|  5 +
> >  ffprobe.c | 11 --
> >  libavformat/dump.c| 10 +
> >  libavutil/spherical.h | 56
> +++
> >  libavutil/version.h   |  2 +-
> >  5 files changed, 81 insertions(+), 3 deletions(-)
> 
>  breaks fate
> 
>  --- ./tests/ref/fate/matroska-spherical-mono2017-02-10
> 23:43:51.993432371 +0100
>  +++ tests/data/fate/matroska-spherical-mono 2017-02-11
> 00:24:10.297483318 +0100
>  @@ -7,7 +7,7 @@
>   [/SIDE_DATA]
>   [SIDE_DATA]
>   side_data_type=Spherical Mapping
>  -side_data_size=16
>  +side_data_size=56
>   projection=equirectangular
>   yaw=45
>   pitch=30
>  Test matroska-spherical-mono failed. Look at
> tests/data/fate/matroska-spherical-mono.err for details.
>  make: *** [fate-matroska-spherical-mono] Error 1
> >>>
> >>> Ah I didn't notice, it is fixed in the next commit, but I'll amend
> this one too.
> >>>
> >>>
> >>> I didn't see any comment/discussion, should I assume it is ok?
> >>> Please CC, thank you.
> >>
> >> These are a lot of projection specific fields. It worries me as the
> >> spec may change in the future with new fields added or existing
> >> fields changing purpose. Not to mention the Mesh projection, which
> >> has like fifty specific fields of its own.
> >
> > Even if the spec change (which at this point would be a terrible
> > terrible thing to do) there are now files in the wild and software
> > that have adopted this draft, so we would have to support this anyway.
>
> If the spec changes, it will be the contents of the equi/cbmp/mesh.
> By exporting them raw as extradata, said changes in the spec would
> require no changes to our implementation.
>

This is one of the main reasons I was suggesting this path. I think of
these extradata fields much like the extra data that codecs have. It really
is only important to the code that needs to render a specific projection.
For transcoding, you mainly just need to convey the information in a
lossless manner from demuxer to muxer.

I anticipate the spec will change in the future. My plan is that no change
will break what is currently specified in the spec right now, but I
anticipate some changes will be made. Having a solution that can gracefully
handle this would be nice.



> >
> >> Wouldn't it be a better idea to export the binary data of the
> >> equi/cbmp/mesh boxes into an extradata-like field in the
> >> AVSphericalMapping struct, and let the downstream application parse
> >> it instead?
> >
> > No I don't think so, lavf is an abstraction layer and one of its tasks
> > is to provide a (simple?) unified entry layer. and letting the user
> > parse binary data is IMO bad design and very fragile. Also it's not
> > impossible that another standard for tagging spherical metadata
> > emerges in the future: the current API could very easily wrap it,
> > while exporting the binary entry would be too specification-specific
> > and it would be tied too heavily on the google draft.
>
>
I agree with Vittorio that having some form of abstraction is a good thing
and having binary data in places can be problematic. It feels like we could
find some middle ground here by providing helper functions that parse the
binary data into projection specific structs and back just like codecs code
tends to do. I feel like this provides a reasonable balance between having
a common set of fields where things actually have common semantics like
projection_type, yaw/pitch/roll, & extra_data while also providing a way to
get access to projection specific information in a simple way.

At the end of the day players really just need to care about a rendering
mesh so in some sense it would be nice to have that be the abstraction for
the player use case. That is basically what we have done in our internal
player implementations. That could easily be handled by helper functions,
but would be a bad representation for AVSphericalMapping because it would
make transcoding/transmuxing painful.


> AVSphericalMapping is already pretty tied to the google draft, but
> i guess you're right, it's at least generic enough for now.
>
>
I too feel like this path is tying the API pretty closely to 

Re: [FFmpeg-devel] [PATCH] matroskaenc: Add support for writing video projection.

[Aaron Colwell]

Given this insistence on using a separate type, the fact that the "tiled
equirect" is under specified, and folks don't actually care about the
"tiled equirect" case right now could I just add code to mux the 2 cases
that are already well specified? I could also send out a patch to fix the
demuxers so they reject the "tiled equirect" cases for now. This seems like
reasonable compromise to allow end-to-end preservation of non-controversial
use cases. That is really all I'm trying to do right now.

Aaron

On Sat, Feb 4, 2017 at 7:46 AM Vittorio Giovara <vittorio.giov...@gmail.com>
wrote:

> On Fri, Feb 3, 2017 at 5:57 PM, Aaron Colwell <acolw...@google.com> wrote:
> > I still think you don't understand what these fields do given what you
> say
> > here. Yes there is a little more math. At the end of the day all these
> > fields do is specify a the min & max for the latitude & longitude. This
> > essentially translates to adding scale factors and offsets in your
> shader or
> > something similar in your 3D geometry creation logic. I get it if
> > implementations don't want to do this small bit of extra work, but saying
> > this is a different type seems strange because you wouldn't do this when
> > talking about cropped 2D images.
>
> If I don't understand these fields, maybe the specification could do a
> better job at explaining what they are for ;)
>
> I am aware that the final projection is the same, but the actual
> problem is that if we don't introduce a new type we would be conveying
> a different semantics to a single projection type. In other words we
> require applications to behave differently according to two different
> fields (the projection type and the offset fields) rather than a
> single one. So yes, the projection is the same, the usecase is
> different, the app behaviour is different, the final processing is
> different -- I think that all this warrants a separate type.
>
> If I still didn't get my point across, let's try with an example: an
> application that does not support the tiled equirectangular
> projection, with a separate type it can immediately discern that it is
> an unsupported type and inform the user, with a single type, instead,
> it has to sort-of-implement the tiling and check for fields that
> should not matter. Another example: look at AVStereo3DType, there is a
> side-by-side and a side-by-side-quincunx : an application that does
> not support quincux can just abort and notify the user, if they were
> two separate fields, you could have applications that do not support
> quincunx but try to render the side-by-side part (which usually
> results in a garbage rendering).
>
> So I reiterate that in my opinion a separate enum value which
> "notifies" apps that they should check additional types is the way to
> go.
>
> >> It is too late to change the spec, but I do believe that the usecase
> >> is different enough to add a new type, in order to not overcomplicate
> >> the implementation.
> >
> >
> > It feels like you are just moving the problem to the demuxers and muxers
> > here. Adding a new type means all demuxers will have to contain logic to
> > generate these different types and all muxers will have to contain logic
> to
> > collapse these types back down to a single value.
>
> Yes, I would a 100% add more logic to the 2 muxers and 2 demuxers that
> implement this spec rather than the thousands applications that
> implement the ffmpeg api. Also the "different logic" is literally an
> "if" case, if not little else.
>
> > I don't really want to keep arguing about this. If folks really want
> > different types then I'll do it just because I want to get reading and
> > writing of metadata working end-to-end. I'd like to make a small request
> to
> > use the term "cropped equirectagular" instead of "tiled equirectangular"
> but
> > I don't feel to strongly about that.
>
> Please don't, "cropped" has entirely different meaning, and it's
> already confusing that you can do bitstream level cropping and surface
> cropping. If you really hate the term "tiled" you could use "bounded
> equirectangular" as it is used in the spec.
>
> > I suppose this is just a difference in style so I don't feel too strongly
> > about it. I was just trying to use unions & structs here to make it a
> little
> > clearer about which fields are expected to be valid and when. The fields
> > seemed to be disjoint sets so I was trying to use language features to
> > convey that.
> >
> > I'd also like to float a potential alternative solution. We c

Re: [FFmpeg-devel] [PATCH] matroskaenc: Add support for writing video projection.

[Aaron Colwell]

On Fri, Feb 3, 2017 at 3:28 AM Vittorio Giovara <vittorio.giov...@gmail.com>
wrote:

> On Thu, Feb 2, 2017 at 9:34 PM, James Almer <jamr...@gmail.com> wrote:
> > On 1/31/2017 12:40 PM, Aaron Colwell wrote:
> >>
> >>
> >> On Tue, Jan 31, 2017 at 2:12 AM Vittorio Giovara <
> vittorio.giov...@gmail.com <mailto:vittorio.giov...@gmail.com>> wrote:
> >>
> >> On Sat, Jan 28, 2017 at 4:13 AM, James Almer <jamr...@gmail.com
> <mailto:jamr...@gmail.com>> wrote:
> >>> On 1/27/2017 11:21 PM, Aaron Colwell wrote:
> >>>> On Fri, Jan 27, 2017 at 5:46 PM James Almer <jamr...@gmail.com
> <mailto:jamr...@gmail.com>> wrote:
> >>>>
> >>>> yeah. I'm a little confused why the demuxing code didn't implement
> this to
> >>>> begin with.
> >>>
> >>> Vittorio (The one that implemented AVSphericalMapping) tried to add
> this at
> >>> first, but then removed it because he wasn't sure if he was doing it
> right.
> >>
> >> Hi,
> >> yes this was included initially but then we found out what those
> >> fields were really for, and I didn't want to make the users get as
> >> confused as we were. As a matter of fact Aaron I mentioned this to you
> >> when I proposed that we probably should have separated the classic
> >> equi projection from the tiled one in the specification, in order to
> >> simplify the implementation.
> >>
> >>
> >> Like I said before, it is not a different projection. It is still
> equirectangular and those parameters just crops the projection. It is very
> simple to just verify that the fields are zero if you don't want to support
> the cropped version.
>
> Hello,
> I'm sorry but I heavily disagree. The tiled equirectangular projection
> is something that cannot be used standalone, you have to do additional
> mathematics and take into account different files or streams to
> generate a "normal" or full-frame equirectangular projection. Having a
> separate type allows to include extensions such as the bounds fields,
> which can be safely ignored by the every user that do not need a tiled
> projection.
>

I still think you don't understand what these fields do given what you say
here. Yes there is a little more math. At the end of the day all these
fields do is specify a the min & max for the latitude & longitude. This
essentially translates to adding scale factors and offsets in your shader
or something similar in your 3D geometry creation logic. I get it if
implementations don't want to do this small bit of extra work, but saying
this is a different type seems strange because you wouldn't do this when
talking about cropped 2D images.


>
> It is too late to change the spec, but I do believe that the usecase
> is different enough to add a new type, in order to not overcomplicate
> the implementation.
>

It feels like you are just moving the problem to the demuxers and muxers
here. Adding a new type means all demuxers will have to contain logic to
generate these different types and all muxers will have to contain logic to
collapse these types back down to a single value.

I don't really want to keep arguing about this. If folks really want
different types then I'll do it just because I want to get reading and
writing of metadata working end-to-end. I'd like to make a small request to
use the term "cropped equirectagular" instead of "tiled equirectangular"
but I don't feel to strongly about that.


>
> >>>>> I know you're the one behind the spec in question, but wouldn't it
> be a
> >>>>> better idea to wait until AVSphericalMapping gets a way to propagate
> this
> >>>>> kind of information before adding support for muxing video projection
> >>>>> elements? Or maybe try to implement it right now...
> >>>>>
> >>>>
> >>>> I'm happy to implement support for the projection specific info. What
> would
> >>>> be the best way to proceed. It seems like I could just place a union
> with
> >>>> projection specific structs in AVSphericalMapping. I'd also like some
> >>>
> >>> I'm CCing Vittorio so he can chim in. I personally have no real
> preference.
> >>
> >> The best way in my opinion is to add a third type, such as
> >> AV_SPHERICAL_TILED_EQUI, and add the relevant fields in
> >> AVSphericalMapping, with a clear description about the actual use case
> >> for them, mentioning that they are used only in format. By the way,
> >> why do you mention adding a union? I think fo

Re: [FFmpeg-devel] [PATCH] matroskaenc: Add support for writing video projection.

[Aaron Colwell]

On Tue, Jan 31, 2017 at 2:12 AM Vittorio Giovara <vittorio.giov...@gmail.com>
wrote:

> On Sat, Jan 28, 2017 at 4:13 AM, James Almer <jamr...@gmail.com> wrote:
> > On 1/27/2017 11:21 PM, Aaron Colwell wrote:
> >> On Fri, Jan 27, 2017 at 5:46 PM James Almer <jamr...@gmail.com> wrote:
> >>
> >> yeah. I'm a little confused why the demuxing code didn't implement this
> to
> >> begin with.
> >
> > Vittorio (The one that implemented AVSphericalMapping) tried to add this
> at
> > first, but then removed it because he wasn't sure if he was doing it
> right.
>
> Hi,
> yes this was included initially but then we found out what those
> fields were really for, and I didn't want to make the users get as
> confused as we were. As a matter of fact Aaron I mentioned this to you
> when I proposed that we probably should have separated the classic
> equi projection from the tiled one in the specification, in order to
> simplify the implementation.
>

Like I said before, it is not a different projection. It is still
equirectangular and those parameters just crops the projection. It is very
simple to just verify that the fields are zero if you don't want to support
the cropped version.


>
> >>> I know you're the one behind the spec in question, but wouldn't it be a
> >>> better idea to wait until AVSphericalMapping gets a way to propagate
> this
> >>> kind of information before adding support for muxing video projection
> >>> elements? Or maybe try to implement it right now...
> >>>
> >>
> >> I'm happy to implement support for the projection specific info. What
> would
> >> be the best way to proceed. It seems like I could just place a union
> with
> >> projection specific structs in AVSphericalMapping. I'd also like some
> >
> > I'm CCing Vittorio so he can chim in. I personally have no real
> preference.
>
> The best way in my opinion is to add a third type, such as
> AV_SPHERICAL_TILED_EQUI, and add the relevant fields in
> AVSphericalMapping, with a clear description about the actual use case
> for them, mentioning that they are used only in format. By the way,
> why do you mention adding a union? I think four plain fields should
> do.
>

I don't think it is worth having the extra enum value for this. All the
cropped fields do is control how you generate the spherical mesh or control
the shader used to render the projection. If players don't want to support
it they can just check to see that all the fields are zero and error out if
not.

I was suggesting using a union because the projection bounds fields are for
equirect, and there are different fields for the cubemap & mesh
projections. I figured that the enum + union of structs might be a
reasonable way to organize the projection specific fields.



>
> Please keep me in CC.
>

Will do.

Aaron


> --
> Vittorio
> ___
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] matroskaenc: Add support for writing video projection.

[Aaron Colwell]

On Fri, Jan 27, 2017 at 5:46 PM James Almer <jamr...@gmail.com> wrote:

> On 1/27/2017 5:12 PM, Aaron Colwell wrote:
> > Adding support for writing spherical metadata in Matroska.
> >
> >
> > 0001-matroskaenc-Add-support-for-writing-video-projection.patch
> >
> >
> > From 5a9cf56bf3114186412bb5572b153f886edb6ddb Mon Sep 17 00:00:00 2001
> > From: Aaron Colwell <acolw...@google.com>
> > Date: Fri, 27 Jan 2017 12:07:25 -0800
> > Subject: [PATCH] matroskaenc: Add support for writing video projection
> >  element.
> >
> > Adding support for writing spherical metadata in Matroska.
> > ---
> >  libavformat/matroskaenc.c | 64
> +++
> >  1 file changed, 64 insertions(+)
> >
> > diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c
> > index f731b678b9..1b186db223 100644
> > --- a/libavformat/matroskaenc.c
> > +++ b/libavformat/matroskaenc.c
> > @@ -1038,6 +1038,67 @@ static int mkv_write_stereo_mode(AVFormatContext
> *s, AVIOContext *pb,
> >  return ret;
> >  }
> >
> > +static int mkv_write_video_projection(AVIOContext *pb, AVStream *st)
> > +{
> > +AVSphericalMapping *spherical_mapping =
> (AVSphericalMapping*)av_stream_get_side_data(st, AV_PKT_DATA_SPHERICAL,
> NULL);
> > +ebml_master projection;
> > +int projection_type = 0;
> > +
> > +AVIOContext *dyn_cp;
> > +uint8_t *projection_private_ptr = NULL;
> > +int ret, projection_private_size;
> > +
> > +if (!spherical_mapping)
> > +return 0;
> > +
> > +if (spherical_mapping->projection != AV_SPHERICAL_EQUIRECTANGULAR &&
> > +spherical_mapping->projection != AV_SPHERICAL_CUBEMAP) {
> > +av_log(pb, AV_LOG_WARNING, "Unsupported projection %d.
> projection not written.\n", spherical_mapping->projection);
> > +return 0;
> > +}
> > +
> > +ret = avio_open_dyn_buf(_cp);
> > +if (ret < 0)
> > +return ret;
> > +
> > +switch (spherical_mapping->projection) {
> > +case AV_SPHERICAL_EQUIRECTANGULAR:
> > +projection_type = 1;
> > +
> > +/* Create ProjectionPrivate contents */
> > +avio_wb32(dyn_cp, 0); /* version = 0 & flags = 0 */
> > +avio_wb32(dyn_cp, 0); /* projection_bounds_top */
> > +avio_wb32(dyn_cp, 0); /* projection_bounds_bottom */
> > +avio_wb32(dyn_cp, 0); /* projection_bounds_left */
> > +avio_wb32(dyn_cp, 0); /* projection_bounds_right */
>
> You could instead use a local 20 byte array, fill it using AV_WB32() and
> then write it with put_ebml_binary().
>

I was mainly using this form since that is what the code would have to look
like once AVSphericalMapping actually contained this data.


>
> Also, the latest change to the spec draft mentions ProjectionPrivate is
> optional for Equirectangular projections if the contents are going to be
> all zero.
>

True. I could just drop this if that is preferred.


>
> > +break;
> > +case AV_SPHERICAL_CUBEMAP:
> > +projection_type = 2;
> > +
> > +/* Create ProjectionPrivate contents */
> > +avio_wb32(dyn_cp, 0); /* version = 0 & flags = 0 */
> > +avio_wb32(dyn_cp, 0); /* layout */
> > +avio_wb32(dyn_cp, 0); /* padding */
> > +break;
> > +}
>
> Same, 12 byte array.
>
> What if the user is trying to remux a matroska file that has a
> ProjectionPrivate element or an mp4 with an equi box filled with non zero
> values, for that matter?
>

yeah. I'm a little confused why the demuxing code didn't implement this to
begin with.


> I know you're the one behind the spec in question, but wouldn't it be a
> better idea to wait until AVSphericalMapping gets a way to propagate this
> kind of information before adding support for muxing video projection
> elements? Or maybe try to implement it right now...
>

I'm happy to implement support for the projection specific info. What would
be the best way to proceed. It seems like I could just place a union with
projection specific structs in AVSphericalMapping. I'd also like some
advice around how to sequence the set of changes. My preference would be to
add the union & fields to AVSphericalMapping and update at least one
demuxer to at least justify the presence of the fields in a single patch.
Not sure if that is the preferred way to go about this though.


>
> This also applies to the mp4 patch.
>

Understood and makes sense.


>
> > +
> &g

[FFmpeg-devel] [PATCH] matroskaenc: Add support for writing video projection.

[Aaron Colwell]

Adding support for writing spherical metadata in Matroska.
From 5a9cf56bf3114186412bb5572b153f886edb6ddb Mon Sep 17 00:00:00 2001
From: Aaron Colwell <acolw...@google.com>
Date: Fri, 27 Jan 2017 12:07:25 -0800
Subject: [PATCH] matroskaenc: Add support for writing video projection
 element.

Adding support for writing spherical metadata in Matroska.
---
 libavformat/matroskaenc.c | 64 +++
 1 file changed, 64 insertions(+)

diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c
index f731b678b9..1b186db223 100644
--- a/libavformat/matroskaenc.c
+++ b/libavformat/matroskaenc.c
@@ -1038,6 +1038,67 @@ static int mkv_write_stereo_mode(AVFormatContext *s, AVIOContext *pb,
 return ret;
 }
 
+static int mkv_write_video_projection(AVIOContext *pb, AVStream *st)
+{
+AVSphericalMapping *spherical_mapping = (AVSphericalMapping*)av_stream_get_side_data(st, AV_PKT_DATA_SPHERICAL, NULL);
+ebml_master projection;
+int projection_type = 0;
+
+AVIOContext *dyn_cp;
+uint8_t *projection_private_ptr = NULL;
+int ret, projection_private_size;
+
+if (!spherical_mapping)
+return 0;
+
+if (spherical_mapping->projection != AV_SPHERICAL_EQUIRECTANGULAR &&
+spherical_mapping->projection != AV_SPHERICAL_CUBEMAP) {
+av_log(pb, AV_LOG_WARNING, "Unsupported projection %d. projection not written.\n", spherical_mapping->projection);
+return 0;
+}
+
+ret = avio_open_dyn_buf(_cp);
+if (ret < 0)
+return ret;
+
+switch (spherical_mapping->projection) {
+case AV_SPHERICAL_EQUIRECTANGULAR:
+projection_type = 1;
+
+/* Create ProjectionPrivate contents */
+avio_wb32(dyn_cp, 0); /* version = 0 & flags = 0 */
+avio_wb32(dyn_cp, 0); /* projection_bounds_top */
+avio_wb32(dyn_cp, 0); /* projection_bounds_bottom */
+avio_wb32(dyn_cp, 0); /* projection_bounds_left */
+avio_wb32(dyn_cp, 0); /* projection_bounds_right */
+break;
+case AV_SPHERICAL_CUBEMAP:
+projection_type = 2;
+
+/* Create ProjectionPrivate contents */
+avio_wb32(dyn_cp, 0); /* version = 0 & flags = 0 */
+avio_wb32(dyn_cp, 0); /* layout */
+avio_wb32(dyn_cp, 0); /* padding */
+break;
+}
+
+projection_private_size = avio_close_dyn_buf(dyn_cp, _private_ptr);
+
+projection = start_ebml_master(pb, MATROSKA_ID_VIDEOPROJECTION, 0);
+put_ebml_uint(pb, MATROSKA_ID_VIDEOPROJECTIONTYPE, projection_type);
+if (projection_private_size)
+put_ebml_binary(pb, MATROSKA_ID_VIDEOPROJECTIONPRIVATE, projection_private_ptr, projection_private_size);
+
+put_ebml_float(pb, MATROSKA_ID_VIDEOPROJECTIONPOSEYAW, (float)(spherical_mapping->yaw) / (1 << 16));
+put_ebml_float(pb, MATROSKA_ID_VIDEOPROJECTIONPOSEPITCH, (float)(spherical_mapping->pitch) / (1 << 16));
+put_ebml_float(pb, MATROSKA_ID_VIDEOPROJECTIONPOSEROLL, (float)(spherical_mapping->roll) / (1 << 16));
+end_ebml_master(pb, projection);
+
+av_free(projection_private_ptr);
+
+return 0;
+}
+
 static int mkv_write_track(AVFormatContext *s, MatroskaMuxContext *mkv,
int i, AVIOContext *pb, int default_stream_exists)
 {
@@ -1251,6 +1312,9 @@ static int mkv_write_track(AVFormatContext *s, MatroskaMuxContext *mkv,
 ret = mkv_write_video_color(pb, par, st);
 if (ret < 0)
 return ret;
+ret = mkv_write_video_projection(pb, st);
+if (ret < 0)
+return ret;
 end_ebml_master(pb, subinfo);
 break;
 
-- 
2.11.0.483.g087da7b7c-goog

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCH] mov: Fix spherical metadata_source parsing.

[Aaron Colwell]

The metadata_source field is a null-terminated string, like other ISOBMFF
strings, not an 8-bit length followed by string characters. This patch
fixes the parsing code so it rejects svhd boxes that are too small and
skips to the end of the svhd box since we don't actually care about the
contents of the
metadata_source field.
From f63f65135e7059376acff3acc0e5268a8861d21d Mon Sep 17 00:00:00 2001
From: Aaron Colwell <acolw...@google.com>
Date: Fri, 27 Jan 2017 09:33:29 -0800
Subject: [PATCH] mov: Fix spherical metadata_source parsing.

The metadata_source field is a null-terminated string, like other ISOBMFF strings,
not an 8-bit length followed by string characters. This patch fixes the parsing
code so it rejects svhd boxes that are too small and skips to the end of the svhd
box since we don't actually care about the contents of the
metadata_source field.
---
 libavformat/mov.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/libavformat/mov.c b/libavformat/mov.c
index 7dc550eb99..b1bfa0a35f 100644
--- a/libavformat/mov.c
+++ b/libavformat/mov.c
@@ -4566,7 +4566,7 @@ static int mov_read_sv3d(MOVContext *c, AVIOContext *pb, MOVAtom atom)
 }
 
 size = avio_rb32(pb);
-if (size > atom.size)
+if (size <= 12 || size > atom.size)
 return AVERROR_INVALIDDATA;
 
 tag = avio_rl32(pb);
@@ -4575,7 +4575,7 @@ static int mov_read_sv3d(MOVContext *c, AVIOContext *pb, MOVAtom atom)
 return 0;
 }
 avio_skip(pb, 4); /*  version + flags */
-avio_skip(pb, avio_r8(pb)); /* metadata_source */
+avio_skip(pb, size - 12); /* metadata_source */
 
 size = avio_rb32(pb);
 if (size > atom.size)
-- 
2.11.0.483.g087da7b7c-goog

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] mov: Fix spherical metadata_source field parsing.

[Aaron Colwell]

On Mon, Jan 9, 2017 at 7:27 PM James Almer <jamr...@gmail.com> wrote:

> On 1/9/2017 11:47 PM, Aaron Colwell wrote:
> > On Mon, Jan 9, 2017 at 6:00 PM James Almer <jamr...@gmail.com> wrote:
> >
> >> On 1/9/2017 3:47 PM, Aaron Colwell wrote:
> >>> The attached patch fixes MOV spherical metadata parsing when the
> >>> metadata_source field is not an empty string.
> >>>
> >>> The metadata_source field is a null-terminated string, like other
> ISOBMFF
> >>> strings,
> >>> not an 8-bit length followed by string characters. This patch fixes the
> >>> parsing
> >>> code so it skips over the string properly.
> >>>
> >>> Aaron
> >>>
> >>>
> >>> 0001-mov-Fix-spherical-metadata_source-field-parsing.patch
> >>>
> >>>
> >>> From a20866dfeae07a5427e8255145f7fe19d846187d Mon Sep 17 00:00:00 2001
> >>> From: Aaron Colwell <acolw...@google.com>
> >>> Date: Mon, 9 Jan 2017 09:58:01 -0800
> >>> Subject: [PATCH] mov: Fix spherical metadata_source field parsing.
> >>>
> >>> The metadata_source field is a null-terminated string like other
> ISOBMFF
> >> strings
> >>> not an 8-bit length followed by string characters. This patch fixes the
> >> parsing
> >>> code so it skips over the string properly.
> >>> ---
> >>>  libavformat/mov.c | 7 ++-
> >>>  1 file changed, 6 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/libavformat/mov.c b/libavformat/mov.c
> >>> index d1b929174d..4399d2ab13 100644
> >>> --- a/libavformat/mov.c
> >>> +++ b/libavformat/mov.c
> >>> @@ -4553,6 +4553,7 @@ static int mov_read_sv3d(MOVContext *c,
> >> AVIOContext *pb, MOVAtom atom)
> >>>  int32_t yaw, pitch, roll;
> >>>  uint32_t tag;
> >>>  enum AVSphericalProjection projection;
> >>> +int i;
> >>>
> >>>  if (c->fc->nb_streams < 1)
> >>>  return 0;
> >>> @@ -4575,7 +4576,11 @@ static int mov_read_sv3d(MOVContext *c,
> >> AVIOContext *pb, MOVAtom atom)
> >>>  return 0;
> >>>  }
> >>>  avio_skip(pb, 4); /*  version + flags */
> >>> -avio_skip(pb, avio_r8(pb)); /* metadata_source */
> >>> +
> >>> +/* metadata_source */
> >>> +for (i = 0; i < size - 12; ++i)
> >>> +if (!avio_r8(pb))
> >>> +break;
> >>
> >> Wouldn't just doing avio_skip(pb, size - 12) be enough for this?
> >>
> >
> > Yes. That would be the smallest change that could possibly work, but
> would
> > be more permissive than what the spec requires. I was trying to fix the
> bug
> > and
> > have bad content trigger an error.
>
> But unless we decide to export it as metadata, we don't currently care
> about
> the contents of the box.
>

True. I was just trying to put in some minimal validation to ensure we
don't permit invalid files. I could use avio_tell() to make sure I consumed
the whole box, but I'm not sure that is heading in the direction you're
hoping for.

FWIW I work on the spec this is implementing so I am bias towards rejecting
non-compliant files.


>
> The size value is expected to be correct in a sane file. If it isn't and if
> we skip said amount of bytes, the code expecting the following box will
> promptly fail.
> With your code, and assuming I'm reading it right and not missing
> something,
> if a null byte is found before the reported svhd box size is fully consumed
> we would in fact be being more permissive by letting the demuxer continue
> if
> the next box effectively starts right after said null byte, regardless of
> what the svhd box size reported.
>

Yes you are correct. I guess this is just exchanging one form of being
permissive for another. :/  I can see your point about just skipping the
box contents might be preferable if the minimal validation above isn't
desirable.

It looks like there are several other assumptions this code is making about
only getting valid boxes. I think the "size > atom.size" checks in several
places might be wrong and aren't taking into account the bytes that have
been consumed at various points. I'll probably try to address that in a
different patch though.


>
> In any case I'm CCing the author of this code to see what he prefers.
>

Ok. Thanks. I appreciate you taking a look at this.

Aaron


>
> >>
> >>>
> >>>  siz

Re: [FFmpeg-devel] [PATCH] mov: Fix spherical metadata_source field parsing.

[Aaron Colwell]

On Mon, Jan 9, 2017 at 6:00 PM James Almer <jamr...@gmail.com> wrote:

> On 1/9/2017 3:47 PM, Aaron Colwell wrote:
> > The attached patch fixes MOV spherical metadata parsing when the
> > metadata_source field is not an empty string.
> >
> > The metadata_source field is a null-terminated string, like other ISOBMFF
> > strings,
> > not an 8-bit length followed by string characters. This patch fixes the
> > parsing
> > code so it skips over the string properly.
> >
> > Aaron
> >
> >
> > 0001-mov-Fix-spherical-metadata_source-field-parsing.patch
> >
> >
> > From a20866dfeae07a5427e8255145f7fe19d846187d Mon Sep 17 00:00:00 2001
> > From: Aaron Colwell <acolw...@google.com>
> > Date: Mon, 9 Jan 2017 09:58:01 -0800
> > Subject: [PATCH] mov: Fix spherical metadata_source field parsing.
> >
> > The metadata_source field is a null-terminated string like other ISOBMFF
> strings
> > not an 8-bit length followed by string characters. This patch fixes the
> parsing
> > code so it skips over the string properly.
> > ---
> >  libavformat/mov.c | 7 ++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/libavformat/mov.c b/libavformat/mov.c
> > index d1b929174d..4399d2ab13 100644
> > --- a/libavformat/mov.c
> > +++ b/libavformat/mov.c
> > @@ -4553,6 +4553,7 @@ static int mov_read_sv3d(MOVContext *c,
> AVIOContext *pb, MOVAtom atom)
> >  int32_t yaw, pitch, roll;
> >  uint32_t tag;
> >  enum AVSphericalProjection projection;
> > +int i;
> >
> >  if (c->fc->nb_streams < 1)
> >  return 0;
> > @@ -4575,7 +4576,11 @@ static int mov_read_sv3d(MOVContext *c,
> AVIOContext *pb, MOVAtom atom)
> >  return 0;
> >  }
> >  avio_skip(pb, 4); /*  version + flags */
> > -avio_skip(pb, avio_r8(pb)); /* metadata_source */
> > +
> > +/* metadata_source */
> > +for (i = 0; i < size - 12; ++i)
> > +if (!avio_r8(pb))
> > +break;
>
> Wouldn't just doing avio_skip(pb, size - 12) be enough for this?
>

Yes. That would be the smallest change that could possibly work, but would
be more permissive than what the spec requires. I was trying to fix the bug
and
have bad content trigger an error.


>
> >
> >  size = avio_rb32(pb);
> >  if (size > atom.size)
> > -- 2.11.0.390.gc69c2f50cf-goog
> >
> >
> >
> > ___
> > ffmpeg-devel mailing list
> > ffmpeg-devel@ffmpeg.org
> > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> >
>
> ___
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCH] mov: Fix spherical metadata_source field parsing.

[Aaron Colwell]

The attached patch fixes MOV spherical metadata parsing when the
metadata_source field is not an empty string.

The metadata_source field is a null-terminated string, like other ISOBMFF
strings,
not an 8-bit length followed by string characters. This patch fixes the
parsing
code so it skips over the string properly.

Aaron
From a20866dfeae07a5427e8255145f7fe19d846187d Mon Sep 17 00:00:00 2001
From: Aaron Colwell <acolw...@google.com>
Date: Mon, 9 Jan 2017 09:58:01 -0800
Subject: [PATCH] mov: Fix spherical metadata_source field parsing.

The metadata_source field is a null-terminated string like other ISOBMFF strings
not an 8-bit length followed by string characters. This patch fixes the parsing
code so it skips over the string properly.
---
 libavformat/mov.c | 7 ++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/libavformat/mov.c b/libavformat/mov.c
index d1b929174d..4399d2ab13 100644
--- a/libavformat/mov.c
+++ b/libavformat/mov.c
@@ -4553,6 +4553,7 @@ static int mov_read_sv3d(MOVContext *c, AVIOContext *pb, MOVAtom atom)
 int32_t yaw, pitch, roll;
 uint32_t tag;
 enum AVSphericalProjection projection;
+int i;
 
 if (c->fc->nb_streams < 1)
 return 0;
@@ -4575,7 +4576,11 @@ static int mov_read_sv3d(MOVContext *c, AVIOContext *pb, MOVAtom atom)
 return 0;
 }
 avio_skip(pb, 4); /*  version + flags */
-avio_skip(pb, avio_r8(pb)); /* metadata_source */
+
+/* metadata_source */
+for (i = 0; i < size - 12; ++i)
+if (!avio_r8(pb))
+break;
 
 size = avio_rb32(pb);
 if (size > atom.size)
-- 
2.11.0.390.gc69c2f50cf-goog

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] Fix sample_aspect_ratio computation for stereo matroska content.

[Aaron Colwell]

On Wed, Dec 2, 2015 at 2:16 AM wm4 <nfx...@googlemail.com> wrote:

> On Wed, 02 Dec 2015 00:32:46 +0000
> Aaron Colwell <acolw...@google.com> wrote:
>
> > On Mon, Nov 30, 2015 at 4:35 PM wm4 <nfx...@googlemail.com> wrote:
> >
> > >
> > > I tried your patch, and it actually makes it work better (looks correct
> > > with the patch). The patch itself also LGTM.
> > >
> > >
> > Ok. Great. Thanks for the review. What do I need to do next to get this
> > checked in. I don't have commit access.
>
> Well, first it has to be determined whether the patch is accepted, and
> then someone has to push it. I just did the latter.
>
> Before pushing, I slightly modified the subject line to add a prefix
> and to make it somewhat shorter (as the prefix made it too long). I
> hope this is ok.
>

Yes. That is fine. Thank you again for your help. I appreciate it.

Aaron
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] Fix sample_aspect_ratio computation for stereo matroska content.

[Aaron Colwell]

On Mon, Nov 30, 2015 at 4:35 PM wm4  wrote:

>
> I tried your patch, and it actually makes it work better (looks correct
> with the patch). The patch itself also LGTM.
>
>
Ok. Great. Thanks for the review. What do I need to do next to get this
checked in. I don't have commit access.

Aaron
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] Fix sample_aspect_ratio computation for stereo matroska content.

[Aaron Colwell]

On Mon, Nov 30, 2015 at 10:57 AM wm4 <nfx...@googlemail.com> wrote:

> On Mon, 30 Nov 2015 18:31:56 +0000
> Aaron Colwell <acolw...@google.com> wrote:
>
> > Hi,
> > Can I get a review for this please? It has been a week since I sent out
> > this patch and I haven't gotten any real feedback about the changes. I've
> > attached a new copy that has been rebased to HEAD. If I need to be doing
> > something different, please let me know so I can make progress on this.
> >
> > Thanks,
> > Aaron
> >
> > On Wed, Nov 25, 2015 at 4:40 PM Aaron Colwell <acolw...@google.com>
> wrote:
> >
> > > On Tue, Nov 24, 2015 at 10:43 AM Kirill Gavrilov <gavr.m...@gmail.com>
> > > wrote:
> > >
> > >> On Mon, Nov 23, 2015 at 11:37 PM, Aaron Colwell <acolw...@google.com>
> > >> wrote:
> > >>
> > >> > matroskaenc.c applies divisors to the display width/height when
> > >> generating
> > >> > stereo content. This patch adds the corresponding multipliers to
> > >> > matroskadec.c
> > >> > so that the original sample aspect ratio can be recovered.
> > >>
> > >>
> > >> Just to link this patch with Matroska Specification notes:
> > >> http://www.matroska.org/technical/specs/notes.html#3D
> > >>
> > >> > The pixel count of the track (PixelWidth/PixelHeight) should be the
> raw
> > >> > amount of pixels (for example 3840x1080 for full HD side by side)
> > >> > and the DisplayWidth/Height in pixels should be the amount of
> pixels for
> > >> > one plane (1920x1080 for that full HD stream).
> > >> >
> > >>
> > >
> > > Is this intended to be a call to action or are you just adding
> information
> > > to this particular thread? I'd like to get the change reviewed so I can
> > > determine whether I need to make any other changes.
> > >
> > > This change basically allows 'ffmpeg -i blah.mkv foo.mkv ; ffprobe
> > > foo.mkv' to show that foo.mkv has the same SAR and DAR as blah.mkv when
> > > blah.mkv contains stereo content. Without this patch the SAR & DAR get
> > > smaller based on whichever dimension is halved by the stereo layout.
> > >
> > > Hope this helps,
> > > Aaron
> > >
> > >
>
> Are you sure matroskadec is wrong and not matroskaenc?
>

Thanks for the response.

Yes. I believe matroskadec is wrong. From what I can tell matroskaenc is
writing the correct information to the file. The issue is that matroskadec
does not appear to be doing the right thing to restore the same state that
was present when the file was written. Specifically sample_aspect_ratio
does not get restored to the value it had during writing.

Here is an example using a SIDE_BY_SIDE_RL file that has
PixelWidthxPixelHeight of "640x360" and a DisplayWidthxDisplayHeight of
"640x360"

./ffmpeg -y -i fp3_RL_640x360.webm  blah.webm && ./ffprobe blah.webm

[ffmpeg output]
Input #0, matroska,webm, from 'fp3_RL_640x360.webm':
  Metadata:
encoder :  google
  Duration: 00:00:04.88, start: 0.00, bitrate: 208 kb/s
Stream #0:0: Video: vp8, yuv420p, 640x360, SAR 1:1 DAR 16:9, 23.98 fps,
23.98 tbr, 1k tbn, 1k tbc (default)
Metadata:
  stereo_mode : right_left
Side data:
  stereo3d: side by side (inverted)
[libvpx-vp9 @ 0x33dcfa0] v1.4.0-729-g8bf791e
Output #0, webm, to 'blah.webm':
  Metadata:
encoder : Lavf57.19.100
Stream #0:0: Video: vp9 (libvpx-vp9), yuv420p, 640x360 [SAR 1:1 DAR
16:9], q=-1--1, 200 kb/s, 23.98 fps, 1k tbn, 23.98 tbc (default)
Metadata:
  stereo_mode : right_left
  encoder : Lavc57.16.101 libvpx-vp9
Side data:
  stereo3d: side by side (inverted)
Stream mapping:
  Stream #0:0 -> #0:0 (vp8 (native) -> vp9 (libvpx-vp9))
[end of ffmpeg output]

--[ffprobe output]--
Input #0, matroska,webm, from 'blah.webm':
  Metadata:
encoder : Lavf57.19.100
  Duration: 00:00:04.88, start: 0.00, bitrate: 100 kb/s
Stream #0:0: Video: vp9 (Profile 0), yuv420p(tv), 640x360, SAR 1:2 DAR
8:9, 23.98 fps, 23.98 tbr, 1k tbn, 1k tbc (default)
Metadata:
  stereo_mode : right_left
Side data:
  stereo3d: side by side (inverted)
--[end of ffprobe output]--

Note that ffprobe SAR and DAR do not match what ffmpeg thought it wrote.
This is what I'm fixing. With my fix, ffprobe reports what ffmpeg said it
wrote.

Here is what the output looks like with my change:

---[ffmpeg output]---
Input #0, matroska,webm, from 'fp3_RL_640x360.webm':
  Metadata:
encoder : google
  Duration: 00:

Re: [FFmpeg-devel] [PATCH] Fix sample_aspect_ratio computation for stereo matroska content.

[Aaron Colwell]

On Tue, Nov 24, 2015 at 10:43 AM Kirill Gavrilov <gavr.m...@gmail.com>
wrote:

> On Mon, Nov 23, 2015 at 11:37 PM, Aaron Colwell <acolw...@google.com>
> wrote:
>
> > matroskaenc.c applies divisors to the display width/height when
> generating
> > stereo content. This patch adds the corresponding multipliers to
> > matroskadec.c
> > so that the original sample aspect ratio can be recovered.
>
>
> Just to link this patch with Matroska Specification notes:
> http://www.matroska.org/technical/specs/notes.html#3D
>
> > The pixel count of the track (PixelWidth/PixelHeight) should be the raw
> > amount of pixels (for example 3840x1080 for full HD side by side)
> > and the DisplayWidth/Height in pixels should be the amount of pixels for
> > one plane (1920x1080 for that full HD stream).
> >
>

Is this intended to be a call to action or are you just adding information
to this particular thread? I'd like to get the change reviewed so I can
determine whether I need to make any other changes.

This change basically allows 'ffmpeg -i blah.mkv foo.mkv ; ffprobe foo.mkv'
to show that foo.mkv has the same SAR and DAR as blah.mkv when blah.mkv
contains stereo content. Without this patch the SAR & DAR get smaller based
on whichever dimension is halved by the stereo layout.

Hope this helps,
Aaron
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCH] Fix sample_aspect_ratio computation for stereo matroska content.

[Aaron Colwell]

matroskaenc.c applies divisors to the display width/height when generating
stereo content. This patch adds the corresponding multipliers to
matroskadec.c
so that the original sample aspect ratio can be recovered.
---
 libavformat/matroskadec.c | 34 --
 1 file changed, 32 insertions(+), 2 deletions(-)

diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
index 424d7bf..f05ddd6 100644
--- a/libavformat/matroskadec.c
+++ b/libavformat/matroskadec.c
@@ -1648,6 +1648,30 @@ static int matroska_parse_flac(AVFormatContext *s,
 return 0;
 }

+static void mkv_stereo_mode_display_mul(int stereo_mode, int *h_width, int
*h_height)
+{
+switch (stereo_mode) {
+case MATROSKA_VIDEO_STEREOMODE_TYPE_MONO:
+case MATROSKA_VIDEO_STEREOMODE_TYPE_CHECKERBOARD_RL:
+case MATROSKA_VIDEO_STEREOMODE_TYPE_CHECKERBOARD_LR:
+case MATROSKA_VIDEO_STEREOMODE_TYPE_BOTH_EYES_BLOCK_RL:
+case MATROSKA_VIDEO_STEREOMODE_TYPE_BOTH_EYES_BLOCK_LR:
+break;
+case MATROSKA_VIDEO_STEREOMODE_TYPE_RIGHT_LEFT:
+case MATROSKA_VIDEO_STEREOMODE_TYPE_LEFT_RIGHT:
+case MATROSKA_VIDEO_STEREOMODE_TYPE_COL_INTERLEAVED_RL:
+case MATROSKA_VIDEO_STEREOMODE_TYPE_COL_INTERLEAVED_LR:
+*h_width = 2;
+break;
+case MATROSKA_VIDEO_STEREOMODE_TYPE_BOTTOM_TOP:
+case MATROSKA_VIDEO_STEREOMODE_TYPE_TOP_BOTTOM:
+case MATROSKA_VIDEO_STEREOMODE_TYPE_ROW_INTERLEAVED_RL:
+case MATROSKA_VIDEO_STEREOMODE_TYPE_ROW_INTERLEAVED_LR:
+*h_height = 2;
+break;
+}
+}
+
 static int matroska_parse_tracks(AVFormatContext *s)
 {
 MatroskaDemuxContext *matroska = s->priv_data;
@@ -2007,6 +2031,8 @@ static int matroska_parse_tracks(AVFormatContext *s)

 if (track->type == MATROSKA_TRACK_TYPE_VIDEO) {
 MatroskaTrackPlane *planes =
track->operation.combine_planes.elem;
+int display_width_mul = 1;
+int display_height_mul = 1;

 st->codec->codec_type = AVMEDIA_TYPE_VIDEO;
 st->codec->codec_tag  = fourcc;
@@ -2014,10 +2040,14 @@ static int matroska_parse_tracks(AVFormatContext *s)
 st->codec->bits_per_coded_sample = bit_depth;
 st->codec->width  = track->video.pixel_width;
 st->codec->height = track->video.pixel_height;
+
+if (track->video.stereo_mode && track->video.stereo_mode <
MATROSKA_VIDEO_STEREOMODE_TYPE_NB)
+mkv_stereo_mode_display_mul(track->video.stereo_mode,
_width_mul, _height_mul);
+
 av_reduce(>sample_aspect_ratio.num,
   >sample_aspect_ratio.den,
-  st->codec->height * track->video.display_width,
-  st->codec->width  * track->video.display_height,
+  st->codec->height * track->video.display_width *
display_width_mul,
+  st->codec->width  * track->video.display_height *
display_height_mul,
   255);
 if (st->codec->codec_id != AV_CODEC_ID_HEVC)
 st->need_parsing = AVSTREAM_PARSE_HEADERS;
-- 
2.6.0.rc2.230.g3dd15c0
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel