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) >>> + >>> + 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 _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel