[FFmpeg-devel] [PATCH] matroskaenc: Add support for writing video projection.
James Almer
jamrial at gmail.com
Sat Jan 28 05:13:15 EET 2017
On 1/27/2017 11:21 PM, Aaron Colwell wrote:
> On Fri, Jan 27, 2017 at 5:46 PM James Almer <jamrial at 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 <acolwell at 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.
I know. I meant doing something like
uint8_t private[20];
AV_WB32(private + 0, 0);
AV_WB32(private + 4, projection_bounds_top);
AV_WB32(private + 8, projection_bounds_bottom);
AV_WB32(private + 12, projection_bounds_left);
AV_WB32(private + 16, projection_bounds_right);
put_ebml_binary(dyn_cp, MATROSKA_ID_VIDEOPROJECTIONPRIVATE, private, sizeof(private));
Then the same for Cubemap, but it's mostly a nit.
>
>
>>
>> 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.
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.
>
>
>> 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.
> 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.
Yeah, one patch to extend AVSphericalMapping, then separate patches to make
use of the new fields in both the mov and matroska demuxers.
>
>
>>
>> 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.
Ah, now that's an oversight. start_ebml_master() there was meant to use
the return value from avio_close_dyn_buf(). I'll fix that later.
The idea is to use a dynamic buffer to write every child element from the
master (In this case Colour or Projection), then dump its contents into the
output AVIOContext. By knowing the entire master's size you can save up to
seven bytes when writing it.
>
>
>>> +
>>> + 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);
Missed it the first time, but these should be cast to double, not float.
>>> +
>>> + 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;
> }
Yes, but preferably make it
if (s->strict_std_compliance <= FF_COMPLIANCE_EXPERIMENTAL) {
ret = mkv_write_video_projection(pb, st);
if (ret < 0)
return ret;
}
>
> 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 at ffmpeg.org
>>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>>
>>
>> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel at ffmpeg.org
>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
More information about the ffmpeg-devel
mailing list