[FFmpeg-devel] [PATCH] avformat/matroskaenc: add support for Spherical Video elements

[Vittorio Giovara]

On Wed, Mar 8, 2017 at 4:05 PM, James Almer <jamrial at gmail.com> wrote:
> On 3/8/2017 5:08 PM, Vittorio Giovara wrote:
>> On Wed, Mar 8, 2017 at 2:46 PM, James Almer <jamrial at gmail.com> wrote:
>>> Signed-off-by: James Almer <jamrial at gmail.com>
>>> ---
>>>  libavformat/matroskaenc.c | 69 +++++++++++++++++++++++++++++++++++++++++++++++
>>>  1 file changed, 69 insertions(+)
>>>
>>> diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c
>>> index 1605f0cafe..0ee927d63e 100644
>>> --- a/libavformat/matroskaenc.c
>>> +++ b/libavformat/matroskaenc.c
>>> @@ -918,6 +918,72 @@ static int mkv_write_video_color(AVIOContext *pb, AVCodecParameters *par, AVStre
>>>      return 0;
>>>  }
>>>
>>> +static int mkv_write_video_projection(AVIOContext *pb, AVStream *st) {
>>> +    int side_data_size = 0;
>>> +    const AVSphericalMapping *spherical =
>>> +        (const AVSphericalMapping*) av_stream_get_side_data(st, AV_PKT_DATA_SPHERICAL,
>>> +                                                            &side_data_size);
>>> +
>>> +    if (side_data_size == sizeof(AVSphericalMapping)) {
>>
>> I don't think you have to check for this (and checking sizeof() of
>> this struct outside lavu breaks ABI), it's enough to check that size
>> != 0
>
> Ok.
>
> You'll probably also have to do the same on libavformat/dump.c, for
> that matter.

This is valid for most side data there, somebody should do a thorough cleanup

>>> +            put_ebml_binary(dyn_cp, MATROSKA_ID_VIDEOPROJECTIONPRIVATE, private, sizeof(private));
>>> +            break;
>>> +        }
>>> +        case AV_SPHERICAL_EQUIRECTANGULAR:
>>> +            put_ebml_uint(dyn_cp, MATROSKA_ID_VIDEOPROJECTIONTYPE,
>>> +                          MATROSKA_VIDEO_PROJECTION_TYPE_EQUIRECTANGULAR);
>>> +            break;
>>> +        case AV_SPHERICAL_CUBEMAP:
>>> +        {
>>> +            uint8_t private[12];
>>> +            put_ebml_uint(dyn_cp, MATROSKA_ID_VIDEOPROJECTIONTYPE,
>>> +                          MATROSKA_VIDEO_PROJECTION_TYPE_CUBEMAP);
>>> +            AV_WB32(private + 0, 0); // version + flags
>>> +            AV_WB32(private + 4, 0); // layout
>>> +            AV_WB32(private + 8, spherical->padding);
>>> +            put_ebml_binary(dyn_cp, MATROSKA_ID_VIDEOPROJECTIONPRIVATE, private, sizeof(private));
>>> +            break;
>>> +        }
>>> +        default:
>>> +            // TODO: Mesh projection once implemented in AVSphericalMapping
>>
>> a little av_log message to warn about this?
>
> This is a muxer, spherical->projection will be one of the supported enums.
> This default case is just to prevent potential compiler warnings and can
> be removed.
>
> What needs a log message is the demuxer.

This is a muxer, that can be created by an API user that does not know
how to read documentation and who initialises spherical->projection
wrong. I didn't say to error out, but I think that adding just a
warning is easier to deal with than having to debug it manually.

>>
>>> +            goto end;
>>> +        }
>>> +
>>> +        put_ebml_float(dyn_cp, MATROSKA_ID_VIDEOPROJECTIONPOSEYAW,   (double)spherical->yaw   / (1 << 16));
>>> +        put_ebml_float(dyn_cp, MATROSKA_ID_VIDEOPROJECTIONPOSEPITCH, (double)spherical->pitch / (1 << 16));
>>> +        put_ebml_float(dyn_cp, MATROSKA_ID_VIDEOPROJECTIONPOSEROLL,  (double)spherical->roll  / (1 << 16));
>>
>> does the matroska spec require plain integers?
>> spherical->{yaw,pitch,roll} are in 16.16 so there should be no need of
>> extra conversions
>
> Matroska stores these three as double precision fp values.

nice
-- 
Vittorio