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

---
Updated according to James' review.
Please CC.
Vittorio

 libavformat/matroskadec.c              | 69 ++++++++++++++++++++++++++++++++--
 tests/ref/fate/matroska-spherical-mono |  6 ++-
 2 files changed, 71 insertions(+), 4 deletions(-)

On 2/15/2017 1:29 PM, Vittorio Giovara wrote:
> ---
> Updated according to James' review.
> Please CC.
> Vittorio
> 
>  libavformat/matroskadec.c              | 69 ++++++++++++++++++++++++++++++++--
>  tests/ref/fate/matroska-spherical-mono |  6 ++-
>  2 files changed, 71 insertions(+), 4 deletions(-)
> 
> diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
> index 7223e94..6fa961e 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;
> +        }

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;

But it's cosmetic. If you change it then don't sent a new patch just for it.

>          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,20 @@ 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) {
> +        /* conversion from 0.32 coordinates to pixels */
> +        size_t orig_width  = (size_t) track->video.pixel_width  * UINT32_MAX / (UINT32_MAX - r - l);
> +        size_t orig_height = (size_t) track->video.pixel_height * UINT32_MAX / (UINT32_MAX - b - t);
> +
> +        /* add a (UINT32_MAX - 1) to round up integer division */
> +        spherical->bound_left   = (orig_width  * l + UINT32_MAX - 1) / UINT32_MAX;
> +        spherical->bound_top    = (orig_height * t + UINT32_MAX - 1) / UINT32_MAX;
> +        spherical->bound_right  = orig_width  - track->video.pixel_width  - spherical->bound_left;
> +        spherical->bound_bottom = orig_height - track->video.pixel_height - spherical->bound_top;
> +    }
> +
>      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

The element parsing code lgtm. No comments about all the added math.