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

[wm4]

On Tue, 7 Mar 2017 21:38:52 +0100
Hendrik Leppkes  wrote:

> On Tue, Mar 7, 2017 at 7:26 PM, wm4  wrote:
> > On Tue, 21 Feb 2017 17:35:53 -0500
> > 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(, track->video.projection.private.data,
> >> + track->video.projection.private.size);
> >> +
> >> +if (bytestream2_get_byte() != 0) {
> >> +av_log(NULL, AV_LOG_WARNING, "Unknown spherical metadata\n");
> >> +return 0;
> >> +}
> >> +
> >> +bytestream2_skip(, 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();
> >> +b = bytestream2_get_be32();
> >> +l = bytestream2_get_be32();
> >> +r = bytestream2_get_be32();
> >> +
> >> +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();
> >> +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();
> >> +} 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

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

[James Almer]

On 2/28/2017 3:06 PM, Vittorio Giovara wrote:
> On Tue, Feb 28, 2017 at 12:46 PM, Vittorio Giovara
>  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();
 +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();
>>>
>>> Nit: Maybe
>>>
>>>if (layout) {
>>>// return error
>>>}
>>>projection = AV_SPHERICAL_CUBEMAP;
>>>padding= bytestream2_get_be32();
> 
> 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?

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[James Almer]

On 3/7/2017 5:38 PM, Hendrik Leppkes wrote:
> On Tue, Mar 7, 2017 at 7:26 PM, wm4  wrote:
>> On Tue, 21 Feb 2017 17:35:53 -0500
>> 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(, track->video.projection.private.data,
>>> + track->video.projection.private.size);
>>> +
>>> +if (bytestream2_get_byte() != 0) {
>>> +av_log(NULL, AV_LOG_WARNING, "Unknown spherical metadata\n");
>>> +return 0;
>>> +}
>>> +
>>> +bytestream2_skip(, 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();
>>> +b = bytestream2_get_be32();
>>> +l = bytestream2_get_be32();
>>> +r = bytestream2_get_be32();
>>> +
>>> +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();
>>> +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();
>>> +} 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, 

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

[Hendrik Leppkes]

On Tue, Mar 7, 2017 at 7:26 PM, wm4  wrote:
> On Tue, 21 Feb 2017 17:35:53 -0500
> 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(, track->video.projection.private.data,
>> + track->video.projection.private.size);
>> +
>> +if (bytestream2_get_byte() != 0) {
>> +av_log(NULL, AV_LOG_WARNING, "Unknown spherical metadata\n");
>> +return 0;
>> +}
>> +
>> +bytestream2_skip(, 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();
>> +b = bytestream2_get_be32();
>> +l = bytestream2_get_be32();
>> +r = bytestream2_get_be32();
>> +
>> +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();
>> +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();
>> +} 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 

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

[wm4]

On Tue, 21 Feb 2017 17:35:53 -0500
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(, track->video.projection.private.data,
> + track->video.projection.private.size);
> +
> +if (bytestream2_get_byte() != 0) {
> +av_log(NULL, AV_LOG_WARNING, "Unknown spherical metadata\n");
> +return 0;
> +}
> +
> +bytestream2_skip(, 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();
> +b = bytestream2_get_be32();
> +l = bytestream2_get_be32();
> +r = bytestream2_get_be32();
> +
> +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();
> +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();
> +} 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?
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[James Almer]

On 2/28/2017 3:21 PM, Vittorio Giovara wrote:
> On Tue, Feb 28, 2017 at 1:15 PM, James Almer  wrote:
>> On 2/28/2017 2:46 PM, Vittorio Giovara wrote:
>>> On Tue, Feb 28, 2017 at 11:00 AM, James Almer  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(, 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() != 0) {
>> +av_log(NULL, AV_LOG_WARNING, "Unknown spherical metadata\n");
>> +return 0;
>> +}
>> +
>> +bytestream2_skip(, 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();
>> +b = bytestream2_get_be32();
>> +l = bytestream2_get_be32();
>> +r = bytestream2_get_be32();
>> +
>> +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..
> 

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Vittorio Giovara]

On Tue, Feb 28, 2017 at 1:15 PM, James Almer  wrote:
> On 2/28/2017 2:46 PM, Vittorio Giovara wrote:
>> On Tue, Feb 28, 2017 at 11:00 AM, James Almer  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(, 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() != 0) {
> +av_log(NULL, AV_LOG_WARNING, "Unknown spherical metadata\n");
> +return 0;
> +}
> +
> +bytestream2_skip(, 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();
> +b = bytestream2_get_be32();
> +l = bytestream2_get_be32();
> +r = bytestream2_get_be32();
> +
> +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..
-- 
Vittorio
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [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  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(, 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() != 0) {
 +av_log(NULL, AV_LOG_WARNING, "Unknown spherical metadata\n");
 +return 0;
 +}
 +
 +bytestream2_skip(, 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();
 +b = bytestream2_get_be32();
 +l = bytestream2_get_be32();
 +r = bytestream2_get_be32();
 +
 +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();
 +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();
>>>
>>> Nit: Maybe
>>>
>>>if (layout) {
>>>// return error
>>>}
>>>projection = AV_SPHERICAL_CUBEMAP;
>>> 

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

[Vittorio Giovara]

On Tue, Feb 28, 2017 at 12:46 PM, Vittorio Giovara
 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
-- 
Vittorio
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [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  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(, 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() != 0) {
>>> +av_log(NULL, AV_LOG_WARNING, "Unknown spherical metadata\n");
>>> +return 0;
>>> +}
>>> +
>>> +bytestream2_skip(, 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();
>>> +b = bytestream2_get_be32();
>>> +l = bytestream2_get_be32();
>>> +r = bytestream2_get_be32();
>>> +
>>> +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();
>>> +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();
>>
>> Nit: Maybe
>>
>>if (layout) {
>>// return error
>>}
>>projection = AV_SPHERICAL_CUBEMAP;
>>padding= bytestream2_get_be32();

ok sure

>>> +} else {
>>> +av_log(NULL, AV_LOG_ERROR, "Unknown spherical metadata\n");
>>>  return AVERROR_INVALIDDATA;
>>> -projection = AV_SPHERICAL_CUBEMAP;
>>> +}
>>>  break;
>>>  default:
>>>  return 0;
>>> @@ 

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

[James Almer]

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(, 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() != 0) {
>> +av_log(NULL, AV_LOG_WARNING, "Unknown spherical metadata\n");
>> +return 0;
>> +}
>> +
>> +bytestream2_skip(, 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();
>> +b = bytestream2_get_be32();
>> +l = bytestream2_get_be32();
>> +r = bytestream2_get_be32();
>> +
>> +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();
>> +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();
> 
> Nit: Maybe
> 
>if (layout) {
>// return error
>}
>projection = AV_SPHERICAL_CUBEMAP;
>padding= bytestream2_get_be32();
> 
>> +} 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;
>> +

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

[James Almer]

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(, 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() != 0) {
> +av_log(NULL, AV_LOG_WARNING, "Unknown spherical metadata\n");
> +return 0;
> +}
> +
> +bytestream2_skip(, 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();
> +b = bytestream2_get_be32();
> +l = bytestream2_get_be32();
> +r = bytestream2_get_be32();
> +
> +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();
> +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();

Nit: Maybe

   if (layout) {
   // return error
   }
   projection = AV_SPHERICAL_CUBEMAP;
   padding= bytestream2_get_be32();

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

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

[Vittorio Giovara]

---
 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(, track->video.projection.private.data,
+ track->video.projection.private.size);
+
+if (bytestream2_get_byte() != 0) {
+av_log(NULL, AV_LOG_WARNING, "Unknown spherical metadata\n");
+return 0;
+}
+
+bytestream2_skip(, 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();
+b = bytestream2_get_be32();
+l = bytestream2_get_be32();
+r = bytestream2_get_be32();
+
+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();
+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();
+} 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
-- 
2.10.0

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel