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

Submitted by James Almer on Sept. 16, 2018, 6:29 p.m.

Details

Message ID 20180916182950.13088-2-jamrial@gmail.com
State New
Headers show

Commit Message

James Almer Sept. 16, 2018, 6:29 p.m.
Signed-off-by: James Almer <jamrial@gmail.com>
---
 libavcodec/libaomdec.c | 4 ++++
 1 file changed, 4 insertions(+)

Comments

Mark Thompson Sept. 17, 2018, 10:52 p.m.
On 16/09/18 19:29, James Almer wrote:
> Signed-off-by: James Almer <jamrial@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.)

>      avctx->color_primaries = img->cp;
>      avctx->colorspace  = img->mc;
>      avctx->color_trc   = img->tc;
> 

LGTM otherwise.

Thanks,

- Mark
James Almer Sept. 17, 2018, 11:11 p.m.
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@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@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>

Patch hide | download patch | download mbox

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];
     avctx->color_primaries = img->cp;
     avctx->colorspace  = img->mc;
     avctx->color_trc   = img->tc;