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

Aaron Colwell acolwell at google.com
Mon Jun 5 22:32:42 EEST 2017


Comments below..

On Mon, Jun 5, 2017 at 12:02 PM Vittorio Giovara <vittorio.giovara at 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 at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Add-spherical_mapping-command-line-argument-to-ffmpe.patch
Type: text/x-patch
Size: 8125 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20170605/39b8f14a/attachment.bin>


More information about the ffmpeg-devel mailing list