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 {
> > int nb_time_bases;
> > SpecifierOpt *enc_time_bases;
> > int nb_enc_time_bases;
> > + SpecifierOpt *spherical_mappings;
> > + int nb_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, &spherical_mapping,
> > + &spherical_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(&spherical_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,
> { &hwaccel_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 },
>
> this part looks ok
>
> > 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", OFFSET(projection), AV_OPT_TYPE_INT,
> > + { .i64 = -1 }, -1, AV_SPHERICAL_EQUIRECTANGULAR_TILE, FLAGS,
> "projection" },
> > + { "equirectangular", "equirectangular projection",
> OFFSET(projection), 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",
> OFFSET(projection), AV_OPT_TYPE_CONST,
> > + { .i64 = AV_SPHERICAL_EQUIRECTANGULAR_TILE }, INT_MIN, INT_MAX,
> FLAGS, "projection" },
> > + { "yaw", "initial yaw orientation in degrees", OFFSET(yaw),
> AV_OPT_TYPE_DOUBLE,
> > + { .dbl = 0.0 }, -180.0, 180.0, FLAGS },
> > + { "pitch", "initial pitch orientation in degrees", OFFSET(pitch),
> AV_OPT_TYPE_DOUBLE,
> > + { .dbl = 0.0 }, -90.0, 90.0, FLAGS },
> > + { "roll", "initial roll orientation in degrees", OFFSET(roll),
> AV_OPT_TYPE_DOUBLE,
> > + { .dbl = 0.0 }, -180.0, 180.0, FLAGS },
> > + { "bound_left", "tiled equirectangular left bound",
> OFFSET(bound_left), AV_OPT_TYPE_INT64,
> > + { .i64 = 0 }, 0, UINT_MAX, FLAGS },
> > + { "bound_top", "tiled equirectangular top bound",
> OFFSET(bound_top), AV_OPT_TYPE_INT64,
> > + { .i64 = 0 }, 0, UINT_MAX, FLAGS },
> > + { "bound_right", "tiled equirectangular right bound",
> OFFSET(bound_right), AV_OPT_TYPE_INT64,
> > + { .i64 = 0 }, 0, UINT_MAX, FLAGS },
> > + { "bound_bottom", "tiled equirectangular bottom bound",
> OFFSET(bound_bottom), AV_OPT_TYPE_INT64,
> > + { .i64 = 0 }, 0, UINT_MAX, FLAGS },
> > + { "padding", "cubemap padding in pixels", OFFSET(padding),
> AV_OPT_TYPE_INT64,
> > + { .i64 = 0 }, 0, UINT_MAX, FLAGS },
> > + { NULL }
> > +};
> > +
> > +static const AVClass av_spherical_mapping_class = {
> > + .class_name = "AVSphericalMapping",
> > + .item_name = spherical_mapping_context_to_name,
> > + .option = spherical_mapping_options,
> > + .version = LIBAVUTIL_VERSION_INT,
> > +};
> > +
> > +int av_spherical_parse_option(const char* opts, AVSphericalMapping
> **spherical_mapping,
> > + size_t *spherical_mapping_size) {
> > + SphericalMappingContext ctx = {
> > + .spherical_class = &av_spherical_mapping_class
> > + };
> > + int ret;
> > +
> > + if (!opts)
> > + return AVERROR(EINVAL);
> > +
> > + av_opt_set_defaults(&ctx);
> > + ret = av_set_options_string(&ctx, opts, "=", ",");
> > + if (ret < 0)
> > + return ret;
> > +
> > + if (ctx.projection == -1) {
> > + av_log(NULL, AV_LOG_ERROR, "projection must be specified\n");
> > + return AVERROR(EINVAL);
> > + }
> > +
> > + if (ctx.padding > 0 && ctx.projection != AV_SPHERICAL_CUBEMAP) {
> > + av_log(NULL, AV_LOG_ERROR, "padding only allowed for
> AV_SPHERICAL_CUBEMAP projection.\n");
> > + return AVERROR(EINVAL);
> > + }
> > +
> > + if ((ctx.bound_left > 0 || ctx.bound_top > 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 {
int nb_time_bases;
SpecifierOpt *enc_time_bases;
int nb_enc_time_bases;
+ SpecifierOpt *spherical_mappings;
+ int nb_spherical_mappings;
} OptionsContext;
typedef struct InputFilter {
diff --git a/ffmpeg_opt.c b/ffmpeg_opt.c
index c997ea8faf..319a350c44 100644
--- a/ffmpeg_opt.c
+++ b/ffmpeg_opt.c
@@ -40,6 +40,7 @@
#include "libavutil/parseutils.h"
#include "libavutil/pixdesc.h"
#include "libavutil/pixfmt.h"
+#include "libavutil/spherical.h"
#define DEFAULT_PASS_LOGFILENAME_PREFIX "ffmpeg2pass"
@@ -1514,12 +1515,130 @@ static void check_streamcopy_filters(OptionsContext *o, AVFormatContext *oc,
}
}
+static int set_spherical_mapping(const char* opt, AVStream* st) {
+ 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 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", OFFSET(projection), 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", OFFSET(projection), AV_OPT_TYPE_CONST,
+ { .i64 = AV_SPHERICAL_EQUIRECTANGULAR_TILE }, INT_MIN, INT_MAX, FLAGS, "projection" },
+ { "yaw", "initial yaw orientation in degrees", OFFSET(yaw), AV_OPT_TYPE_DOUBLE,
+ { .dbl = 0.0 }, -180.0, 180.0, FLAGS },
+ { "pitch", "initial pitch orientation in degrees", OFFSET(pitch), AV_OPT_TYPE_DOUBLE,
+ { .dbl = 0.0 }, -90.0, 90.0, FLAGS },
+ { "roll", "initial roll orientation in degrees", OFFSET(roll), AV_OPT_TYPE_DOUBLE,
+ { .dbl = 0.0 }, -180.0, 180.0, FLAGS },
+ { "bound_left", "tiled equirectangular left bound", OFFSET(bound_left), AV_OPT_TYPE_INT64,
+ { .i64 = 0 }, 0, UINT_MAX, FLAGS },
+ { "bound_top", "tiled equirectangular top bound", OFFSET(bound_top), AV_OPT_TYPE_INT64,
+ { .i64 = 0 }, 0, UINT_MAX, FLAGS },
+ { "bound_right", "tiled equirectangular right bound", OFFSET(bound_right), AV_OPT_TYPE_INT64,
+ { .i64 = 0 }, 0, UINT_MAX, FLAGS },
+ { "bound_bottom", "tiled equirectangular bottom bound", OFFSET(bound_bottom), AV_OPT_TYPE_INT64,
+ { .i64 = 0 }, 0, UINT_MAX, FLAGS },
+ { "padding", "cubemap padding in pixels", OFFSET(padding), AV_OPT_TYPE_INT64,
+ { .i64 = 0 }, 0, UINT_MAX, FLAGS },
+ { NULL }
+ };
+#undef OFFSET
+#undef FLAGS
+
+ static const AVClass spherical_mapping_class = {
+ .class_name = "",
+ .item_name = av_default_item_name,
+ .option = opts,
+ .version = LIBAVUTIL_VERSION_INT,
+ };
+
+ SphericalMappingContext ctx = {
+ .spherical_class = &spherical_mapping_class
+ };
+
+ size_t spherical_mapping_size = 0;
+ AVSphericalMapping *spherical_mapping = NULL;
+ int ret;
+
+ if (!opt)
+ return AVERROR(EINVAL);
+
+ av_opt_set_defaults(&ctx);
+ ret = av_set_options_string(&ctx, opt, "=", ",");
+ if (ret < 0)
+ return ret;
+
+ if (ctx.projection == -1) {
+ av_log(NULL, AV_LOG_ERROR, "projection must be specified\n");
+ return AVERROR(EINVAL);
+ }
+
+ if (ctx.padding > 0 && ctx.projection != AV_SPHERICAL_CUBEMAP) {
+ av_log(NULL, AV_LOG_ERROR, "padding only allowed for AV_SPHERICAL_CUBEMAP projection.\n");
+ return AVERROR(EINVAL);
+ }
+
+ if ((ctx.bound_left > 0 || ctx.bound_top > 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;
+ }
+
+ if (ret >= 0) {
+ ret = av_stream_add_side_data(st, AV_PKT_DATA_SPHERICAL, spherical_mapping, spherical_mapping_size);
+
+ if (ret < 0) {
+ av_freep(&spherical_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 +1665,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 +3694,10 @@ const OptionDef options[] = {
"automatically insert correct rotate filters" },
{ "hwaccel_lax_profile_check", OPT_BOOL | OPT_EXPERT, { &hwaccel_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 },
--
2.13.0.506.g27d5fe0cd-goog
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
