diff mbox

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

Message ID 20170221223553.65520-3-vittorio.giovara@gmail.com
State Accepted
Headers show

Commit Message

Vittorio Giovara Feb. 21, 2017, 10:35 p.m. UTC 
---
 libavformat/matroskadec.c              | 64 ++++++++++++++++++++++++++++++++--
 tests/ref/fate/matroska-spherical-mono |  6 +++-
 2 files changed, 66 insertions(+), 4 deletions(-)

Comments

James Almer Feb. 22, 2017, 4:21 p.m. UTC | #1 
On 2/21/2017 7:35 PM, Vittorio Giovara wrote:
> ---
>  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.

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

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

>      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);

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

> +    }
> +
>      ret = av_stream_add_side_data(st, AV_PKT_DATA_SPHERICAL, (uint8_t *)spherical,
>                                    spherical_size);
>      if (ret < 0) {
> diff --git a/tests/ref/fate/matroska-spherical-mono b/tests/ref/fate/matroska-spherical-mono
> index 8048aff..a70d879 100644
> --- a/tests/ref/fate/matroska-spherical-mono
> +++ b/tests/ref/fate/matroska-spherical-mono
> @@ -8,7 +8,11 @@ inverted=0
>  [SIDE_DATA]
>  side_data_type=Spherical Mapping
>  side_data_size=56
> -projection=equirectangular
> +projection=tiled equirectangular
> +bound_left=148
> +bound_top=73
> +bound_right=147
> +bound_bottom=72
>  yaw=45
>  pitch=30
>  roll=15
>
James Almer Feb. 28, 2017, 4 p.m. UTC | #2 
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.
> 
>> +
>> +    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.
> 
>> +                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
        }
        break;
---

> 
>>      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);
> 
>> +        } 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.
> 
>> +    }
>> +
>>      ret = av_stream_add_side_data(st, AV_PKT_DATA_SPHERICAL, (uint8_t *)spherical,
>>                                    spherical_size);
>>      if (ret < 0) {
>> diff --git a/tests/ref/fate/matroska-spherical-mono b/tests/ref/fate/matroska-spherical-mono
>> index 8048aff..a70d879 100644
>> --- a/tests/ref/fate/matroska-spherical-mono
>> +++ b/tests/ref/fate/matroska-spherical-mono
>> @@ -8,7 +8,11 @@ inverted=0
>>  [SIDE_DATA]
>>  side_data_type=Spherical Mapping
>>  side_data_size=56
>> -projection=equirectangular
>> +projection=tiled equirectangular
>> +bound_left=148
>> +bound_top=73
>> +bound_right=147
>> +bound_bottom=72
>>  yaw=45
>>  pitch=30
>>  roll=15
>>
>
Vittorio Giovara Feb. 28, 2017, 5:46 p.m. UTC | #3 
On Tue, Feb 28, 2017 at 11:00 AM, James Almer <jamrial@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 Giovara Feb. 28, 2017, 6:06 p.m. UTC | #4 
On Tue, Feb 28, 2017 at 12:46 PM, Vittorio Giovara
<vittorio.giovara@gmail.com> wrote:
>> ----
>> 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?

nevermind, i get it, and ok
James Almer Feb. 28, 2017, 6:15 p.m. UTC | #5 
On 2/28/2017 2:46 PM, Vittorio Giovara wrote:
> On Tue, Feb 28, 2017 at 11:00 AM, James Almer <jamrial@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
>
Vittorio Giovara Feb. 28, 2017, 6:21 p.m. UTC | #6 
On Tue, Feb 28, 2017 at 1:15 PM, James Almer <jamrial@gmail.com> wrote:
> On 2/28/2017 2:46 PM, Vittorio Giovara wrote:
>> On Tue, Feb 28, 2017 at 11:00 AM, James Almer <jamrial@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.

For now what should we do? Just check that private.size and
private.data are non-0 and return with an error otherwise?

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

old software..............
James Almer Feb. 28, 2017, 7:45 p.m. UTC | #7 
On 2/28/2017 3:21 PM, Vittorio Giovara wrote:
> On Tue, Feb 28, 2017 at 1:15 PM, James Almer <jamrial@gmail.com> wrote:
>> On 2/28/2017 2:46 PM, Vittorio Giovara wrote:
>>> On Tue, Feb 28, 2017 at 11:00 AM, James Almer <jamrial@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.
> 
> For now what should we do? Just check that private.size and
> private.data are non-0 and return with an error otherwise?

Actually, nevermind. The assert check is for size >= 0, so it's just
checking for negative values (probably fuzzing related). Sorry for
the noise.

The code should be ok, as you're using the checked versions of the
read functions.

> 
>>>>>> +    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.
> 
> old software..............
>
wm4 March 7, 2017, 6:26 p.m. UTC | #8 
On Tue, 21 Feb 2017 17:35:53 -0500
Vittorio Giovara <vittorio.giovara@gmail.com> wrote:

> ---
>  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);
> +
> +    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);
> +                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;
>      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);
> +        } 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;
> +    }
> +
>      ret = av_stream_add_side_data(st, AV_PKT_DATA_SPHERICAL, (uint8_t *)spherical,
>                                    spherical_size);
>      if (ret < 0) {
> diff --git a/tests/ref/fate/matroska-spherical-mono b/tests/ref/fate/matroska-spherical-mono
> index 8048aff..a70d879 100644
> --- a/tests/ref/fate/matroska-spherical-mono
> +++ b/tests/ref/fate/matroska-spherical-mono
> @@ -8,7 +8,11 @@ inverted=0
>  [SIDE_DATA]
>  side_data_type=Spherical Mapping
>  side_data_size=56
> -projection=equirectangular
> +projection=tiled equirectangular
> +bound_left=148
> +bound_top=73
> +bound_right=147
> +bound_bottom=72
>  yaw=45
>  pitch=30
>  roll=15

Is it just me, or did this break FATE?
Hendrik Leppkes March 7, 2017, 8:38 p.m. UTC | #9 
On Tue, Mar 7, 2017 at 7:26 PM, wm4 <nfxjfg@googlemail.com> wrote:
> On Tue, 21 Feb 2017 17:35:53 -0500
> Vittorio Giovara <vittorio.giovara@gmail.com> wrote:
>
>> ---
>>  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);
>> +
>> +    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);
>> +                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;
>>      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);
>> +        } 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;
>> +    }
>> +
>>      ret = av_stream_add_side_data(st, AV_PKT_DATA_SPHERICAL, (uint8_t *)spherical,
>>                                    spherical_size);
>>      if (ret < 0) {
>> diff --git a/tests/ref/fate/matroska-spherical-mono b/tests/ref/fate/matroska-spherical-mono
>> index 8048aff..a70d879 100644
>> --- a/tests/ref/fate/matroska-spherical-mono
>> +++ b/tests/ref/fate/matroska-spherical-mono
>> @@ -8,7 +8,11 @@ inverted=0
>>  [SIDE_DATA]
>>  side_data_type=Spherical Mapping
>>  side_data_size=56
>> -projection=equirectangular
>> +projection=tiled equirectangular
>> +bound_left=148
>> +bound_top=73
>> +bound_right=147
>> +bound_bottom=72
>>  yaw=45
>>  pitch=30
>>  roll=15
>
> Is it just me, or did this break FATE?

It broke on some systems because it  prints the side_data_size, which
varies on some compilers/systems since the struct uses size_t
We could probably just remove side_data_size, its not very informative
and as seen here can even be compiler/platform dependent.

- Hendrik
James Almer March 7, 2017, 9:34 p.m. UTC | #10 
On 3/7/2017 5:38 PM, Hendrik Leppkes wrote:
> On Tue, Mar 7, 2017 at 7:26 PM, wm4 <nfxjfg@googlemail.com> wrote:
>> On Tue, 21 Feb 2017 17:35:53 -0500
>> Vittorio Giovara <vittorio.giovara@gmail.com> wrote:
>>
>>> ---
>>>  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);
>>> +
>>> +    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);
>>> +                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;
>>>      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);
>>> +        } 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;
>>> +    }
>>> +
>>>      ret = av_stream_add_side_data(st, AV_PKT_DATA_SPHERICAL, (uint8_t *)spherical,
>>>                                    spherical_size);
>>>      if (ret < 0) {
>>> diff --git a/tests/ref/fate/matroska-spherical-mono b/tests/ref/fate/matroska-spherical-mono
>>> index 8048aff..a70d879 100644
>>> --- a/tests/ref/fate/matroska-spherical-mono
>>> +++ b/tests/ref/fate/matroska-spherical-mono
>>> @@ -8,7 +8,11 @@ inverted=0
>>>  [SIDE_DATA]
>>>  side_data_type=Spherical Mapping
>>>  side_data_size=56
>>> -projection=equirectangular
>>> +projection=tiled equirectangular
>>> +bound_left=148
>>> +bound_top=73
>>> +bound_right=147
>>> +bound_bottom=72
>>>  yaw=45
>>>  pitch=30
>>>  roll=15
>>
>> Is it just me, or did this break FATE?
> 
> It broke on some systems because it  prints the side_data_size, which
> varies on some compilers/systems since the struct uses size_t
> We could probably just remove side_data_size, its not very informative
> and as seen here can even be compiler/platform dependent.
> 
> - Hendrik

Or just make the fields uint32_t. They are supposed to be 32 bytes anyway.
James Almer March 7, 2017, 9:54 p.m. UTC | #11 
On 2/28/2017 3:06 PM, Vittorio Giovara wrote:
> On Tue, Feb 28, 2017 at 12:46 PM, Vittorio Giovara
> <vittorio.giovara@gmail.com> wrote:
>>> ----
>>> 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?
> 
> nevermind, i get it, and ok
> 
>>>>      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

You pushed these two chunks without any of the cosmetic changes i
suggested. You did apply them on libav, though. Do you mind doing it
here as well, or should i?
wm4 March 8, 2017, 6:51 a.m. UTC | #12 
On Tue, 7 Mar 2017 21:38:52 +0100
Hendrik Leppkes <h.leppkes@gmail.com> wrote:

> On Tue, Mar 7, 2017 at 7:26 PM, wm4 <nfxjfg@googlemail.com> wrote:
> > On Tue, 21 Feb 2017 17:35:53 -0500
> > Vittorio Giovara <vittorio.giovara@gmail.com> wrote:
> >  
> >> ---
> >>  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);
> >> +
> >> +    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);
> >> +                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;
> >>      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);
> >> +        } 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;
> >> +    }
> >> +
> >>      ret = av_stream_add_side_data(st, AV_PKT_DATA_SPHERICAL, (uint8_t *)spherical,
> >>                                    spherical_size);
> >>      if (ret < 0) {
> >> diff --git a/tests/ref/fate/matroska-spherical-mono b/tests/ref/fate/matroska-spherical-mono
> >> index 8048aff..a70d879 100644
> >> --- a/tests/ref/fate/matroska-spherical-mono
> >> +++ b/tests/ref/fate/matroska-spherical-mono
> >> @@ -8,7 +8,11 @@ inverted=0
> >>  [SIDE_DATA]
> >>  side_data_type=Spherical Mapping
> >>  side_data_size=56
> >> -projection=equirectangular
> >> +projection=tiled equirectangular
> >> +bound_left=148
> >> +bound_top=73
> >> +bound_right=147
> >> +bound_bottom=72
> >>  yaw=45
> >>  pitch=30
> >>  roll=15  
> >
> > Is it just me, or did this break FATE?  
> 
> It broke on some systems because it  prints the side_data_size, which
> varies on some compilers/systems since the struct uses size_t
> We could probably just remove side_data_size, its not very informative
> and as seen here can even be compiler/platform dependent.

It broke much more here than just the size field.
diff mbox

Patch

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);
+
+    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);
+                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;
     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);
+        } 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;
+    }
+
     ret = av_stream_add_side_data(st, AV_PKT_DATA_SPHERICAL, (uint8_t *)spherical,
                                   spherical_size);
     if (ret < 0) {
diff --git a/tests/ref/fate/matroska-spherical-mono b/tests/ref/fate/matroska-spherical-mono
index 8048aff..a70d879 100644
--- a/tests/ref/fate/matroska-spherical-mono
+++ b/tests/ref/fate/matroska-spherical-mono
@@ -8,7 +8,11 @@  inverted=0
 [SIDE_DATA]
 side_data_type=Spherical Mapping
 side_data_size=56
-projection=equirectangular
+projection=tiled equirectangular
+bound_left=148
+bound_top=73
+bound_right=147
+bound_bottom=72
 yaw=45
 pitch=30
 roll=15