[FFmpeg-devel] [PATCH 2/2] avcodec/libaomdec: export chroma sample location

[James Almer]

On 9/17/2018 7:52 PM, Mark Thompson wrote:
> On 16/09/18 19:29, James Almer wrote:
>> Signed-off-by: James Almer <jamrial at gmail.com>
>> ---
>>  libavcodec/libaomdec.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/libavcodec/libaomdec.c b/libavcodec/libaomdec.c
>> index 2530c9f76b..a21cace164 100644
>> --- a/libavcodec/libaomdec.c
>> +++ b/libavcodec/libaomdec.c
>> @@ -89,7 +89,11 @@ static int set_pix_fmt(AVCodecContext *avctx, struct aom_image *img)
>>      static const enum AVColorRange color_ranges[] = {
>>          AVCOL_RANGE_MPEG, AVCOL_RANGE_JPEG
>>      };
>> +    static const enum AVColorRange chroma_locations[] = {
>> +        AVCHROMA_LOC_UNSPECIFIED, AVCHROMA_LOC_LEFT, AVCHROMA_LOC_TOPLEFT, AVCHROMA_LOC_UNSPECIFIED
>> +    };
>>      avctx->color_range = color_ranges[img->range];
>> +    avctx->chroma_sample_location = chroma_locations[img->csp];
> 
> I would suggest for future compatibility that it probably shouldn't invoke undefined behaviour if the value is not in the range 0-3?
> 
> (While I assume the limited choice of locations here was deliberate, the fact that you can't encode a sequence of JPEGs without resampling seems slightly crazy to me so more being added in future would not be a surprise.)

It's unlikely they'll ever implement anything like that. Even assigning
something for the value 3 (like center as you said), which is currently
reserved, might require a new version of the bitstream defining new
profiles. Not to mention adding even more values, which would require
changing the Sequence Header to make the field wider than two bits.

I personally don't really see that happening, but I'll add a img->csp <=
AOM_CSP_COLOCATED check anyway.

> 
>>      avctx->color_primaries = img->cp;
>>      avctx->colorspace  = img->mc;
>>      avctx->color_trc   = img->tc;
>>
> 
> LGTM otherwise.
> 
> Thanks,
> 
> - Mark
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>