On 2/28/2017 2:46 PM, Vittorio Giovara wrote:
> On Tue, Feb 28, 2017 at 11:00 AM, James Almer <jamr...@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
>
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
