[FFmpeg-devel] [PATCH 2/3] avformat/matroskadec: support parsing Chroma Location elements

[James Almer]

On 10/17/2016 10:00 PM, Michael Niedermayer wrote:
> On Sat, Oct 15, 2016 at 12:40:55PM -0300, James Almer wrote:
>> Signed-off-by: James Almer <jamrial at gmail.com>
>> ---
>>  libavformat/matroskadec.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
>> index acf1ccb..cfe4692 100644
>> --- a/libavformat/matroskadec.c
>> +++ b/libavformat/matroskadec.c
>> @@ -1826,6 +1826,10 @@ static int mkv_parse_video_color(AVStream *st, const MatroskaTrack *track) {
>>      if (track->video.color.range != AVCOL_RANGE_UNSPECIFIED &&
>>          track->video.color.range <= AVCOL_RANGE_JPEG)
>>          st->codecpar->color_range = track->video.color.range;
>> +    if (track->video.color.chroma_siting_horz && track->video.color.chroma_siting_vert)
>> +        st->codecpar->chroma_location =
>> +            avcodec_chroma_pos_to_enum((track->video.color.chroma_siting_horz - 1) << 7,
>> +                                       (track->video.color.chroma_siting_vert - 1) << 7);
> 
> does this need some validity check ? (i didnt immedeatly see any check)
> << 7 could overflow without check

avcodec_chroma_pos_to_enum() will return AVCHROMA_LOC_UNSPECIFIED if
either xpos or ypos have bad values, so even if it overflows it wouldn't
be a problem.
The spec doesn't specify a range for the elements, but currently values
0 (Unspecified), 1 (Left/Top Collocated) and 2 (Half) are the only valid
ones. I guess 3 may be added in the future, at least for ChromaSitingVert
to represent bottom based on our AVChromaLocation enum values.

In any case, I'll add some range checks for both elements to be safe.

> 
> [...]
> 
> 
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>