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(&dyn_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.
>
> > +
> > + projection_private_size = avio_close_dyn_buf(dyn_cp,
> &projection_private_ptr);
>
> The dynbuf should ideally contain the whole Projection master, so you can
> then pass its size to start_ebml_master() instead of zero.
> See mkv_write_video_color().
>
>
I'm a little confused about what you mean by passing the size to
start_ebml_master() it looks like both the calls I see in
mkv_write_video_color() pass in zero.
> > +
> > + 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);
>
> This needs to be inside a check for strict experimental compliance
> nonetheless.
>
Ok. I'm assuming you mean adding something like
if (s->strict_std_compliance > FF_COMPLIANCE_EXPERIMENTAL) {
av_log(s, AV_LOG_ERROR,
"Spherical metadata in Matroska support is experimental,
add "
"'-strict %d' if you want to use it.\n",
FF_COMPLIANCE_EXPERIMENTAL);
return AVERROR_EXPERIMENTAL;
}
Thanks for your help.
Aaron
>
> > + 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 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
