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

[James Almer]

On 3/8/2017 7:09 PM, Vittorio Giovara wrote:
> 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.

Alright, will add one before pushing.

> 
>>>
>>>> +            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
>