[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(&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
>>
>