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

[James Almer]

On 2/28/2017 2:46 PM, Vittorio Giovara wrote:
> 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)

True, guess it should return an error value instead.

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

We also support 2012 and 2013. SIZE_SPECIFIER becomes zu for gcc and
such, but Iu for MSVC.

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