| Message ID | 20180916182950.13088-2-jamrial@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
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
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 >
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;
Signed-off-by: James Almer <jamrial@gmail.com> --- libavcodec/libaomdec.c | 4 ++++ 1 file changed, 4 insertions(+)