[FFmpeg-devel] [PATCHv3 3/3] mkv: Export bounds and padding from spherical metadata

[Vittorio Giovara]

On Tue, Feb 28, 2017 at 11:00 AM, James Almer <jamrial at gmail.com> wrote:
> On 2/22/2017 1:21 PM, James Almer wrote:
>> On 2/21/2017 7:35 PM, Vittorio Giovara wrote:
>
> CCing this one as well.
>
>>> ---
>>>  libavformat/matroskadec.c              | 64 ++++++++++++++++++++++++++++++++--
>>>  tests/ref/fate/matroska-spherical-mono |  6 +++-
>>>  2 files changed, 66 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
>>> index 7223e94..0a02237 100644
>>> --- a/libavformat/matroskadec.c
>>> +++ b/libavformat/matroskadec.c
>>> @@ -1913,16 +1913,65 @@ static int mkv_parse_video_projection(AVStream *st, const MatroskaTrack *track)
>>>      AVSphericalMapping *spherical;
>>>      enum AVSphericalProjection projection;
>>>      size_t spherical_size;
>>> +    size_t l, t, r, b;
>>> +    size_t padding = 0;
>>>      int ret;
>>> +    GetByteContext gb;
>>> +
>>> +    bytestream2_init(&gb, track->video.projection.private.data,
>>> +                     track->video.projection.private.size);
>>
>> private.size can be zero and private.data NULL. bytestream2_init()
>> will trigger an assert in those cases.

:( asserts like this are really dampening, it should be on read not on
creation (if at all)

>>> +
>>> +    if (bytestream2_get_byte(&gb) != 0) {
>>> +        av_log(NULL, AV_LOG_WARNING, "Unknown spherical metadata\n");
>>> +        return 0;
>>> +    }
>>> +
>>> +    bytestream2_skip(&gb, 3); // flags
>>>
>>>      switch (track->video.projection.type) {
>>>      case MATROSKA_VIDEO_PROJECTION_TYPE_EQUIRECTANGULAR:
>>> -        projection = AV_SPHERICAL_EQUIRECTANGULAR;
>>> +        if (track->video.projection.private.size == 0)
>>> +            projection = AV_SPHERICAL_EQUIRECTANGULAR;
>>> +        else if (track->video.projection.private.size == 20) {
>>> +            t = bytestream2_get_be32(&gb);
>>> +            b = bytestream2_get_be32(&gb);
>>> +            l = bytestream2_get_be32(&gb);
>>> +            r = bytestream2_get_be32(&gb);
>>> +
>>> +            if (b >= UINT_MAX - t || r >= UINT_MAX - l) {
>>> +                av_log(NULL, AV_LOG_ERROR,
>>> +                       "Invalid bounding rectangle coordinates "
>>> +                       "%zu,%zu,%zu,%zu\n", l, t, r, b);
>>
>> Use SIZE_SPECIFIER instead of zu. Msvc doesn't like the latter.
>> Same with the mov implementation.

Umh? Isn't %zu supported on any msvc version that matters (2015 onwards)?
https://connect.microsoft.com/VisualStudio/feedback/details/2083820/printf-format-specifier-zu-is-supported-but-not-documented

>>> +                return AVERROR_INVALIDDATA;
>>> +            }
>>> +
>>> +            if (l || t || r || b)
>>> +                projection = AV_SPHERICAL_EQUIRECTANGULAR_TILE;
>>> +            else
>>> +                projection = AV_SPHERICAL_EQUIRECTANGULAR;
>>> +        } else {
>>> +            av_log(NULL, AV_LOG_ERROR, "Unknown spherical metadata\n");
>>> +            return AVERROR_INVALIDDATA;
>>> +        }
>>>          break;
>>
>> Nit: I still think the Equi case looks better the way i suggested in
>> my previous email.
>
> And what i wrote in said previous email that i also didn't CC:
>
> ----
> I think this'll look better as
>
>
>     case MATROSKA_VIDEO_PROJECTION_TYPE_EQUIRECTANGULAR:
>         projection = AV_SPHERICAL_EQUIRECTANGULAR;
>
>         if (track->video.projection.private.size == 20) {
>             [...]
>             if (l || t || r || b)
>                 projection = AV_SPHERICAL_EQUIRECTANGULAR_TILE;
>         } else if (track->video.projection.private.size != 0) {
>             // return error
>         }

Sorry, I don't follow, what is your suggestion?

> ---
>
>>
>>>      case MATROSKA_VIDEO_PROJECTION_TYPE_CUBEMAP:
>>> -        if (track->video.projection.private.size < 4)
>>> +        if (track->video.projection.private.size < 4) {
>>> +            av_log(NULL, AV_LOG_ERROR, "Missing projection private properties\n");
>>> +            return AVERROR_INVALIDDATA;
>>> +        } else if (track->video.projection.private.size == 12) {
>>> +            uint32_t layout = bytestream2_get_be32(&gb);
>>> +            if (layout == 0) {
>>> +                projection = AV_SPHERICAL_CUBEMAP;
>>> +            } else {
>>> +                av_log(NULL, AV_LOG_WARNING,
>>> +                       "Unknown spherical cubemap layout %"PRIu32"\n", layout);
>>> +                return 0;
>>> +            }
>>> +            padding = bytestream2_get_be32(&gb);
>>
>> Nit: Maybe
>>
>>                if (layout) {
>>                    // return error
>>                }
>>                projection = AV_SPHERICAL_CUBEMAP;
>>                padding    = bytestream2_get_be32(&gb);

ok sure

>>> +        } else {
>>> +            av_log(NULL, AV_LOG_ERROR, "Unknown spherical metadata\n");
>>>              return AVERROR_INVALIDDATA;
>>> -        projection = AV_SPHERICAL_CUBEMAP;
>>> +        }
>>>          break;
>>>      default:
>>>          return 0;
>>> @@ -1937,6 +1986,15 @@ static int mkv_parse_video_projection(AVStream *st, const MatroskaTrack *track)
>>>      spherical->pitch = (int32_t)(track->video.projection.pitch * (1 << 16));
>>>      spherical->roll  = (int32_t)(track->video.projection.roll  * (1 << 16));
>>>
>>> +    spherical->padding = padding;
>>> +
>>> +    if (spherical->projection == AV_SPHERICAL_EQUIRECTANGULAR_TILE) {
>>> +        spherical->bound_left   = l;
>>> +        spherical->bound_top    = t;
>>> +        spherical->bound_right  = r;
>>> +        spherical->bound_bottom = b;
>>
>> These four could also be zero initialized so you don't need to check
>> the projection.

ok thank you
-- 
Vittorio