| Message ID | 20210829164323.62565-2-jeebjp@gmail.com |
|---|---|
| State | Accepted |
| Commit | 9b2281a4a383677ded522e603514789cc22498fa |
| Headers | show |
| Series | [FFmpeg-devel,1/2] avcodec/libx265: only call av_pix_fmt_desc_get once in init | expand |
| Context | Check | Description |
|---|---|---|
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
| andriy/make_ppc | success | Make finished |
| andriy/make_fate_ppc | success | Make fate finished |
On Sun, Aug 29, 2021 at 7:43 PM Jan Ekström <jeebjp@gmail.com> wrote: > > Unlike libx264, libx265 does not handle the chroma format check > on its own side, so in order to not write out values which are > supposed to be ignored according to the specification, we limit > the writing out of chroma sample location to 4:2:0 only. > --- > libavcodec/libx265.c | 13 +++++++++++++ > 1 file changed, 13 insertions(+) > > diff --git a/libavcodec/libx265.c b/libavcodec/libx265.c > index 71affbf61b..839b6ce9de 100644 > --- a/libavcodec/libx265.c > +++ b/libavcodec/libx265.c > @@ -211,6 +211,19 @@ static av_cold int libx265_encode_init(AVCodecContext *avctx) > ctx->params->vui.matrixCoeffs = avctx->colorspace; > } > > + // chroma sample location values are to be ignored in case of non-4:2:0 > + // according to the specification, so we only write them out in case of > + // 4:2:0 (log2_chroma_{w,h} == 1). > + ctx->params->vui.bEnableChromaLocInfoPresentFlag = > + avctx->chroma_sample_location != AVCHROMA_LOC_UNSPECIFIED && > + desc->log2_chroma_w == 1 && desc->log2_chroma_h == 1; Checked this another time, and this check seems correct. HWAccel pix_fmts like VAAPI are also marked as 4:2:0, but thankfully they're not on the supported list for this module. For the source of the check, it is based on the following in the specification: Otherwise (chroma_format_idc is not equal to 1), the values of the syntax elements chroma_sample_loc_type_top_field and chroma_sample_loc_type_bottom_field shall be ignored. When chroma_format_idc is equal to 2 (4:2:2 chroma format) or 3 (4:4:4 chroma format), the location of chroma samples is specified in clause 6.2. When chroma_format_idc is equal to 0, there is no chroma sample array. > + > + if (ctx->params->vui.bEnableChromaLocInfoPresentFlag) { > + ctx->params->vui.chromaSampleLocTypeTopField = > + ctx->params->vui.chromaSampleLocTypeBottomField = > + avctx->chroma_sample_location - 1; > + } For progressive content both values should be set to the same value. For interlaced content in theory the chroma location field value is not necessarily the same, but: 1. we have a single value in our APIs 2. x264 does this exact same thing, utilizes a single value interface and sets both fields to the same value. Jan
On Tue, Aug 31, 2021 at 12:52 AM Jan Ekström <jeebjp@gmail.com> wrote: > > On Sun, Aug 29, 2021 at 7:43 PM Jan Ekström <jeebjp@gmail.com> wrote: > > > > Unlike libx264, libx265 does not handle the chroma format check > > on its own side, so in order to not write out values which are > > supposed to be ignored according to the specification, we limit > > the writing out of chroma sample location to 4:2:0 only. > > --- > > libavcodec/libx265.c | 13 +++++++++++++ > > 1 file changed, 13 insertions(+) > > > > diff --git a/libavcodec/libx265.c b/libavcodec/libx265.c > > index 71affbf61b..839b6ce9de 100644 > > --- a/libavcodec/libx265.c > > +++ b/libavcodec/libx265.c > > @@ -211,6 +211,19 @@ static av_cold int libx265_encode_init(AVCodecContext *avctx) > > ctx->params->vui.matrixCoeffs = avctx->colorspace; > > } > > > > + // chroma sample location values are to be ignored in case of non-4:2:0 > > + // according to the specification, so we only write them out in case of > > + // 4:2:0 (log2_chroma_{w,h} == 1). > > + ctx->params->vui.bEnableChromaLocInfoPresentFlag = > > + avctx->chroma_sample_location != AVCHROMA_LOC_UNSPECIFIED && > > + desc->log2_chroma_w == 1 && desc->log2_chroma_h == 1; > > Checked this another time, and this check seems correct. HWAccel > pix_fmts like VAAPI are also marked as 4:2:0, but thankfully they're > not on the supported list for this module. > > For the source of the check, it is based on the following in the specification: > > Otherwise (chroma_format_idc is not equal to 1), the values > of the syntax elements chroma_sample_loc_type_top_field and > chroma_sample_loc_type_bottom_field shall be ignored. When > chroma_format_idc is equal to 2 (4:2:2 chroma format) or 3 (4:4:4 > chroma format), the location of chroma samples is specified in clause > 6.2. When chroma_format_idc is equal to 0, there is no chroma sample > array. > > > + > > + if (ctx->params->vui.bEnableChromaLocInfoPresentFlag) { > > + ctx->params->vui.chromaSampleLocTypeTopField = > > + ctx->params->vui.chromaSampleLocTypeBottomField = > > + avctx->chroma_sample_location - 1; > > + } > > For progressive content both values should be set to the same value. > > For interlaced content in theory the chroma location field value is > not necessarily the same, but: > 1. we have a single value in our APIs > 2. x264 does this exact same thing, utilizes a single value interface > and sets both fields to the same value. Unless there are objections, I will pull this in soon. Jan
On Wed, Sep 1, 2021 at 12:11 AM Jan Ekström <jeebjp@gmail.com> wrote: > > On Tue, Aug 31, 2021 at 12:52 AM Jan Ekström <jeebjp@gmail.com> wrote: > > > > On Sun, Aug 29, 2021 at 7:43 PM Jan Ekström <jeebjp@gmail.com> wrote: > > > > > > Unlike libx264, libx265 does not handle the chroma format check > > > on its own side, so in order to not write out values which are > > > supposed to be ignored according to the specification, we limit > > > the writing out of chroma sample location to 4:2:0 only. > > > --- > > > libavcodec/libx265.c | 13 +++++++++++++ > > > 1 file changed, 13 insertions(+) > > > > > > diff --git a/libavcodec/libx265.c b/libavcodec/libx265.c > > > index 71affbf61b..839b6ce9de 100644 > > > --- a/libavcodec/libx265.c > > > +++ b/libavcodec/libx265.c > > > @@ -211,6 +211,19 @@ static av_cold int libx265_encode_init(AVCodecContext *avctx) > > > ctx->params->vui.matrixCoeffs = avctx->colorspace; > > > } > > > > > > + // chroma sample location values are to be ignored in case of non-4:2:0 > > > + // according to the specification, so we only write them out in case of > > > + // 4:2:0 (log2_chroma_{w,h} == 1). > > > + ctx->params->vui.bEnableChromaLocInfoPresentFlag = > > > + avctx->chroma_sample_location != AVCHROMA_LOC_UNSPECIFIED && > > > + desc->log2_chroma_w == 1 && desc->log2_chroma_h == 1; > > > > Checked this another time, and this check seems correct. HWAccel > > pix_fmts like VAAPI are also marked as 4:2:0, but thankfully they're > > not on the supported list for this module. > > > > For the source of the check, it is based on the following in the specification: > > > > Otherwise (chroma_format_idc is not equal to 1), the values > > of the syntax elements chroma_sample_loc_type_top_field and > > chroma_sample_loc_type_bottom_field shall be ignored. When > > chroma_format_idc is equal to 2 (4:2:2 chroma format) or 3 (4:4:4 > > chroma format), the location of chroma samples is specified in clause > > 6.2. When chroma_format_idc is equal to 0, there is no chroma sample > > array. > > > > > + > > > + if (ctx->params->vui.bEnableChromaLocInfoPresentFlag) { > > > + ctx->params->vui.chromaSampleLocTypeTopField = > > > + ctx->params->vui.chromaSampleLocTypeBottomField = > > > + avctx->chroma_sample_location - 1; > > > + } > > > > For progressive content both values should be set to the same value. > > > > For interlaced content in theory the chroma location field value is > > not necessarily the same, but: > > 1. we have a single value in our APIs > > 2. x264 does this exact same thing, utilizes a single value interface > > and sets both fields to the same value. > > Unless there are objections, I will pull this in soon. Applied as 9b2281a4a383677ded522e603514789cc22498fa . Jan
diff --git a/libavcodec/libx265.c b/libavcodec/libx265.c index 71affbf61b..839b6ce9de 100644 --- a/libavcodec/libx265.c +++ b/libavcodec/libx265.c @@ -211,6 +211,19 @@ static av_cold int libx265_encode_init(AVCodecContext *avctx) ctx->params->vui.matrixCoeffs = avctx->colorspace; } + // chroma sample location values are to be ignored in case of non-4:2:0 + // according to the specification, so we only write them out in case of + // 4:2:0 (log2_chroma_{w,h} == 1). + ctx->params->vui.bEnableChromaLocInfoPresentFlag = + avctx->chroma_sample_location != AVCHROMA_LOC_UNSPECIFIED && + desc->log2_chroma_w == 1 && desc->log2_chroma_h == 1; + + if (ctx->params->vui.bEnableChromaLocInfoPresentFlag) { + ctx->params->vui.chromaSampleLocTypeTopField = + ctx->params->vui.chromaSampleLocTypeBottomField = + avctx->chroma_sample_location - 1; + } + if (avctx->sample_aspect_ratio.num > 0 && avctx->sample_aspect_ratio.den > 0) { char sar[12]; int sar_num, sar_den;