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

Signed-off-by: James Almer <jamrial@gmail.com>
---
 libavformat/matroska.h    | 14 ++++++++++++++
 libavformat/matroskadec.c |  7 +++++++
 2 files changed, 21 insertions(+)

On Tue, Oct 18, 2016 at 12:18:01AM -0300, James Almer wrote:
> Signed-off-by: James Almer <jamrial@gmail.com>
> ---
>  libavformat/matroska.h    | 14 ++++++++++++++
>  libavformat/matroskadec.c |  7 +++++++
>  2 files changed, 21 insertions(+)
> 
> diff --git a/libavformat/matroska.h b/libavformat/matroska.h
> index 8ad89da..13155e5 100644
> --- a/libavformat/matroska.h
> +++ b/libavformat/matroska.h
> @@ -317,6 +317,20 @@ typedef enum {
>    MATROSKA_VIDEO_DISPLAYUNIT_UNKNOWN     = 4,
>  } MatroskaVideoDisplayUnit;
>  
> +typedef enum {
> +  MATROSKA_COLOUR_CHROMASITINGHORZ_UNDETERMINED     = 0,
> +  MATROSKA_COLOUR_CHROMASITINGHORZ_LEFT             = 1,
> +  MATROSKA_COLOUR_CHROMASITINGHORZ_HALF             = 2,
> +  MATROSKA_COLOUR_CHROMASITINGHORZ_NB
> +} MatroskaColourChromaSitingHorz;
> +
> +typedef enum {
> +  MATROSKA_COLOUR_CHROMASITINGVERT_UNDETERMINED     = 0,
> +  MATROSKA_COLOUR_CHROMASITINGVERT_TOP              = 1,
> +  MATROSKA_COLOUR_CHROMASITINGVERT_HALF             = 2,
> +  MATROSKA_COLOUR_CHROMASITINGVERT_NB
> +} MatroskaColourChromaSitingVert;
> +
>  /*
>   * Matroska Codec IDs, strings
>   */
> diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
> index a5d3c0e..722d0b0 100644
> --- a/libavformat/matroskadec.c
> +++ b/libavformat/matroskadec.c
> @@ -1827,6 +1827,13 @@ 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 &&
> +        track->video.color.chroma_siting_horz < MATROSKA_COLOUR_CHROMASITINGHORZ_NB &&
> +        track->video.color.chroma_siting_vert < MATROSKA_COLOUR_CHROMASITINGVERT_NB) {
> +        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);
> +    }

on the assumtation that the cases match up correctly, (I did not
cross-check the spec), LGTM

thx

[...]

On 10/18/2016 11:10 AM, Michael Niedermayer wrote:
> On Tue, Oct 18, 2016 at 12:18:01AM -0300, James Almer wrote:
>> Signed-off-by: James Almer <jamrial@gmail.com>
>> ---
>>  libavformat/matroska.h    | 14 ++++++++++++++
>>  libavformat/matroskadec.c |  7 +++++++
>>  2 files changed, 21 insertions(+)
>>
>> diff --git a/libavformat/matroska.h b/libavformat/matroska.h
>> index 8ad89da..13155e5 100644
>> --- a/libavformat/matroska.h
>> +++ b/libavformat/matroska.h
>> @@ -317,6 +317,20 @@ typedef enum {
>>    MATROSKA_VIDEO_DISPLAYUNIT_UNKNOWN     = 4,
>>  } MatroskaVideoDisplayUnit;
>>  
>> +typedef enum {
>> +  MATROSKA_COLOUR_CHROMASITINGHORZ_UNDETERMINED     = 0,
>> +  MATROSKA_COLOUR_CHROMASITINGHORZ_LEFT             = 1,
>> +  MATROSKA_COLOUR_CHROMASITINGHORZ_HALF             = 2,
>> +  MATROSKA_COLOUR_CHROMASITINGHORZ_NB
>> +} MatroskaColourChromaSitingHorz;
>> +
>> +typedef enum {
>> +  MATROSKA_COLOUR_CHROMASITINGVERT_UNDETERMINED     = 0,
>> +  MATROSKA_COLOUR_CHROMASITINGVERT_TOP              = 1,
>> +  MATROSKA_COLOUR_CHROMASITINGVERT_HALF             = 2,
>> +  MATROSKA_COLOUR_CHROMASITINGVERT_NB
>> +} MatroskaColourChromaSitingVert;
>> +
>>  /*
>>   * Matroska Codec IDs, strings
>>   */
>> diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
>> index a5d3c0e..722d0b0 100644
>> --- a/libavformat/matroskadec.c
>> +++ b/libavformat/matroskadec.c
>> @@ -1827,6 +1827,13 @@ 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 &&
>> +        track->video.color.chroma_siting_horz < MATROSKA_COLOUR_CHROMASITINGHORZ_NB &&
>> +        track->video.color.chroma_siting_vert < MATROSKA_COLOUR_CHROMASITINGVERT_NB) {
>> +        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);
>> +    }
> 
> on the assumtation that the cases match up correctly, (I did not
> cross-check the spec)

The values in the enums above are taken from the spec.

In any case this code is still marked as "unofficial", even though all
the elements are already listed in the spec page and git repo.
Could someone more familiar with the current standardization process
chime in to confirm if these Colour elements are finalized or not? The
unofficial compliance check could be removed if so.

> , LGTM
> thx

Pushed, thanks.