| Message ID | 20201102224649.2563498-1-sw@jkqxz.net |
|---|---|
| State | Accepted |
| Commit | 9faf4dcf234dbc34f05cd2d973473df2eece4881 |
| Headers | show |
| Series | [FFmpeg-devel,1/3] vaapi_encode_h264: Fix setting colour properties | expand |
| Context | Check | Description |
|---|---|---|
| andriy/x86_make | success | Make finished |
| andriy/x86_make_fate | success | Make fate finished |
On Tue, Nov 3, 2020 at 12:59 AM Mark Thompson <sw@jkqxz.net> wrote: > > The properties should always be set; only the presence flags want to be > conditional. > > Fixes #8959. > --- Thanks for this on such short notice. Patch set LGTM, and simplifies the logic nicely for these values (always set, only set flags if required). Tested the H.264 and MPEG-2 encoders locally with my X230, with and without additional color space info with the general command mentioned in #8959. Unfortunately I lack the hardware to test HEVC for now. Jan
> -----Original Message----- > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Jan Ekström > Sent: Tuesday, November 03, 2020 3:17 AM > To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org> > Subject: Re: [FFmpeg-devel] [PATCH 1/3] vaapi_encode_h264: Fix setting colour properties > > On Tue, Nov 3, 2020 at 12:59 AM Mark Thompson <sw@jkqxz.net> wrote: > > > > The properties should always be set; only the presence flags want to be > > conditional. > > > > Fixes #8959. > > --- > > Thanks for this on such short notice. Patch set LGTM, and simplifies > the logic nicely for these values (always set, only set flags if > required). > > Tested the H.264 and MPEG-2 encoders locally with my X230, with and > without additional color space info with the general command mentioned > in #8959. Unfortunately I lack the hardware to test HEVC for now. > > Jan Thanks. Also... Tested-By: Xu, Yefeng <yefengx.xu@intel.com> ...whom also tested these patches with one of our quick internal test suites covering 170 AVC, 73 HEVC and 30 MPEG2 cases on an ICL platform. With the patches, tests are fixed and no new regressions detected. Regards, U. Artie > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
On 03/11/2020 16:51, Eoff, Ullysses A wrote:>> -----Original Message----- >> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Jan Ekström >> Sent: Tuesday, November 03, 2020 3:17 AM >> To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org> >> Subject: Re: [FFmpeg-devel] [PATCH 1/3] vaapi_encode_h264: Fix setting colour properties >> >> On Tue, Nov 3, 2020 at 12:59 AM Mark Thompson <sw@jkqxz.net> wrote: >>> >>> The properties should always be set; only the presence flags want to be >>> conditional. >>> >>> Fixes #8959. >>> --- >> >> Thanks for this on such short notice. Patch set LGTM, and simplifies >> the logic nicely for these values (always set, only set flags if >> required). >> >> Tested the H.264 and MPEG-2 encoders locally with my X230, with and >> without additional color space info with the general command mentioned >> in #8959. Unfortunately I lack the hardware to test HEVC for now. >> >> Jan > > Thanks. Also... > > Tested-By: Xu, Yefeng <yefengx.xu@intel.com> > > ...whom also tested these patches with one of our quick internal test > suites covering 170 AVC, 73 HEVC and 30 MPEG2 cases on an ICL > platform. With the patches, tests are fixed and no new regressions > detected. Sounds good! Applied. Thanks, - Mark
diff --git a/libavcodec/vaapi_encode_h264.c b/libavcodec/vaapi_encode_h264.c index 5e1683e851..e52a0e37a4 100644 --- a/libavcodec/vaapi_encode_h264.c +++ b/libavcodec/vaapi_encode_h264.c @@ -411,30 +411,20 @@ static int vaapi_encode_h264_init_sequence_params(AVCodecContext *avctx) sps->vui.aspect_ratio_info_present_flag = 1; } - if (avctx->color_range != AVCOL_RANGE_UNSPECIFIED || - avctx->color_primaries != AVCOL_PRI_UNSPECIFIED || + // Unspecified video format, from table E-2. + sps->vui.video_format = 5; + sps->vui.video_full_range_flag = + avctx->color_range == AVCOL_RANGE_JPEG; + sps->vui.colour_primaries = avctx->color_primaries; + sps->vui.transfer_characteristics = avctx->color_trc; + sps->vui.matrix_coefficients = avctx->colorspace; + if (avctx->color_primaries != AVCOL_PRI_UNSPECIFIED || avctx->color_trc != AVCOL_TRC_UNSPECIFIED || - avctx->colorspace != AVCOL_SPC_UNSPECIFIED) { + avctx->colorspace != AVCOL_SPC_UNSPECIFIED) + sps->vui.colour_description_present_flag = 1; + if (avctx->color_range != AVCOL_RANGE_UNSPECIFIED || + sps->vui.colour_description_present_flag) sps->vui.video_signal_type_present_flag = 1; - sps->vui.video_format = 5; // Unspecified. - sps->vui.video_full_range_flag = - avctx->color_range == AVCOL_RANGE_JPEG; - - if (avctx->color_primaries != AVCOL_PRI_UNSPECIFIED || - avctx->color_trc != AVCOL_TRC_UNSPECIFIED || - avctx->colorspace != AVCOL_SPC_UNSPECIFIED) { - sps->vui.colour_description_present_flag = 1; - sps->vui.colour_primaries = avctx->color_primaries; - sps->vui.transfer_characteristics = avctx->color_trc; - sps->vui.matrix_coefficients = avctx->colorspace; - } - } else { - sps->vui.video_format = 5; - sps->vui.video_full_range_flag = 0; - sps->vui.colour_primaries = avctx->color_primaries; - sps->vui.transfer_characteristics = avctx->color_trc; - sps->vui.matrix_coefficients = avctx->colorspace; - } if (avctx->chroma_sample_location != AVCHROMA_LOC_UNSPECIFIED) { sps->vui.chroma_loc_info_present_flag = 1;