From: Lynne <dev@lynne.ee>
To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>
Subject: Re: [FFmpeg-devel] [PATCH 1/2] avcodec/hevc_ps: fix setting HEVCHdrParams fields
Date: Thu, 21 Mar 2024 18:14:17 +0100 (CET)
Message-ID: <NtX-Oyf--3-9@lynne.ee> (raw)
In-Reply-To: <20240320231730.27666-1-jamrial@gmail.com>
Mar 21, 2024, 00:17 by jamrial@gmail.com:
> These were defined in a way compatible with the Vulkan HEVC acceleration, which
> expects bitmasks, yet the fields were being overwritting on each loop with the
> latest read value.
>
> Signed-off-by: James Almer <jamrial@gmail.com>
> ---
> libavcodec/hevc_ps.c | 44 ++++++++++++++++++++++------------------
> libavcodec/hevc_ps.h | 15 +++++++-------
> libavcodec/vulkan_hevc.c | 16 +++++++--------
> 3 files changed, 40 insertions(+), 35 deletions(-)
>
> diff --git a/libavcodec/hevc_ps.c b/libavcodec/hevc_ps.c
> index fb997066d9..20ceb09829 100644
> --- a/libavcodec/hevc_ps.c
> +++ b/libavcodec/hevc_ps.c
> @@ -370,7 +370,7 @@ static void decode_sublayer_hrd(GetBitContext *gb, unsigned int nb_cpb,
> par->bit_rate_du_value_minus1[i] = get_ue_golomb_long(gb);
> }
>
> - par->cbr_flag = get_bits1(gb);
> + par->cbr_flag |= get_bits1(gb) << i;
> }
> }
>
> @@ -378,24 +378,24 @@ static int decode_hrd(GetBitContext *gb, int common_inf_present,
> HEVCHdrParams *hdr, int max_sublayers)
> {
> if (common_inf_present) {
> - hdr->flags.nal_hrd_parameters_present_flag = get_bits1(gb);
> - hdr->flags.vcl_hrd_parameters_present_flag = get_bits1(gb);
> + hdr->nal_hrd_parameters_present_flag = get_bits1(gb);
> + hdr->vcl_hrd_parameters_present_flag = get_bits1(gb);
>
> - if (hdr->flags.nal_hrd_parameters_present_flag ||
> - hdr->flags.vcl_hrd_parameters_present_flag) {
> - hdr->flags.sub_pic_hrd_params_present_flag = get_bits1(gb);
> + if (hdr->nal_hrd_parameters_present_flag ||
> + hdr->vcl_hrd_parameters_present_flag) {
> + hdr->sub_pic_hrd_params_present_flag = get_bits1(gb);
>
> - if (hdr->flags.sub_pic_hrd_params_present_flag) {
> + if (hdr->sub_pic_hrd_params_present_flag) {
> hdr->tick_divisor_minus2 = get_bits(gb, 8);
> hdr->du_cpb_removal_delay_increment_length_minus1 = get_bits(gb, 5);
> - hdr->flags.sub_pic_cpb_params_in_pic_timing_sei_flag = get_bits1(gb);
> + hdr->sub_pic_cpb_params_in_pic_timing_sei_flag = get_bits1(gb);
> hdr->dpb_output_delay_du_length_minus1 = get_bits(gb, 5);
> }
>
> hdr->bit_rate_scale = get_bits(gb, 4);
> hdr->cpb_size_scale = get_bits(gb, 4);
>
> - if (hdr->flags.sub_pic_hrd_params_present_flag)
> + if (hdr->sub_pic_hrd_params_present_flag)
> hdr->cpb_size_du_scale = get_bits(gb, 4);
>
> hdr->initial_cpb_removal_delay_length_minus1 = get_bits(gb, 5);
> @@ -405,18 +405,22 @@ static int decode_hrd(GetBitContext *gb, int common_inf_present,
> }
>
> for (int i = 0; i < max_sublayers; i++) {
> - hdr->flags.fixed_pic_rate_general_flag = get_bits1(gb);
> + unsigned fixed_pic_rate_general_flag = get_bits1(gb);
> + unsigned fixed_pic_rate_within_cvs_flag = 0;
> + unsigned low_delay_hrd_flag = 0;
> + hdr->flags.fixed_pic_rate_general_flag |= fixed_pic_rate_general_flag << i;
>
> - if (!hdr->flags.fixed_pic_rate_general_flag)
> - hdr->flags.fixed_pic_rate_within_cvs_flag = get_bits1(gb);
> + if (!fixed_pic_rate_general_flag)
> + fixed_pic_rate_within_cvs_flag = get_bits1(gb);
> + hdr->flags.fixed_pic_rate_within_cvs_flag |= fixed_pic_rate_within_cvs_flag << i;
>
> - if (hdr->flags.fixed_pic_rate_within_cvs_flag ||
> - hdr->flags.fixed_pic_rate_general_flag)
> + if (fixed_pic_rate_within_cvs_flag || fixed_pic_rate_general_flag)
> hdr->elemental_duration_in_tc_minus1[i] = get_ue_golomb_long(gb);
> else
> - hdr->flags.low_delay_hrd_flag = get_bits1(gb);
> + low_delay_hrd_flag = get_bits1(gb);
> + hdr->flags.low_delay_hrd_flag |= low_delay_hrd_flag << i;
>
> - if (!hdr->flags.low_delay_hrd_flag) {
> + if (!low_delay_hrd_flag) {
> unsigned cpb_cnt_minus1 = get_ue_golomb_long(gb);
> if (cpb_cnt_minus1 > 31) {
> av_log(NULL, AV_LOG_ERROR, "nb_cpb %d invalid\n",
> @@ -426,13 +430,13 @@ static int decode_hrd(GetBitContext *gb, int common_inf_present,
> hdr->cpb_cnt_minus1[i] = cpb_cnt_minus1;
> }
>
> - if (hdr->flags.nal_hrd_parameters_present_flag)
> + if (hdr->nal_hrd_parameters_present_flag)
> decode_sublayer_hrd(gb, hdr->cpb_cnt_minus1[i]+1, &hdr->nal_params[i],
> - hdr->flags.sub_pic_hrd_params_present_flag);
> + hdr->sub_pic_hrd_params_present_flag);
>
> - if (hdr->flags.vcl_hrd_parameters_present_flag)
> + if (hdr->vcl_hrd_parameters_present_flag)
> decode_sublayer_hrd(gb, hdr->cpb_cnt_minus1[i]+1, &hdr->vcl_params[i],
> - hdr->flags.sub_pic_hrd_params_present_flag);
> + hdr->sub_pic_hrd_params_present_flag);
> }
>
> return 0;
> diff --git a/libavcodec/hevc_ps.h b/libavcodec/hevc_ps.h
> index 786c896709..88d6f617b5 100644
> --- a/libavcodec/hevc_ps.h
> +++ b/libavcodec/hevc_ps.h
> @@ -39,18 +39,19 @@ typedef struct HEVCSublayerHdrParams {
> uint32_t cbr_flag;
> } HEVCSublayerHdrParams;
>
> +// flags in bitmask form
> typedef struct HEVCHdrFlagParams {
> - uint32_t nal_hrd_parameters_present_flag;
> - uint32_t vcl_hrd_parameters_present_flag;
> - uint32_t sub_pic_hrd_params_present_flag;
> - uint32_t sub_pic_cpb_params_in_pic_timing_sei_flag;
> - uint32_t fixed_pic_rate_general_flag;
> - uint32_t fixed_pic_rate_within_cvs_flag;
> - uint32_t low_delay_hrd_flag;
> + uint8_t fixed_pic_rate_general_flag;
> + uint8_t fixed_pic_rate_within_cvs_flag;
> + uint8_t low_delay_hrd_flag;
> } HEVCHdrFlagParams;
>
> typedef struct HEVCHdrParams {
> HEVCHdrFlagParams flags;
> + uint8_t nal_hrd_parameters_present_flag;
> + uint8_t vcl_hrd_parameters_present_flag;
> + uint8_t sub_pic_hrd_params_present_flag;
> + uint8_t sub_pic_cpb_params_in_pic_timing_sei_flag;
>
> uint8_t tick_divisor_minus2;
> uint8_t du_cpb_removal_delay_increment_length_minus1;
> diff --git a/libavcodec/vulkan_hevc.c b/libavcodec/vulkan_hevc.c
> index e2acc35612..239bff75e5 100644
> --- a/libavcodec/vulkan_hevc.c
> +++ b/libavcodec/vulkan_hevc.c
> @@ -250,10 +250,10 @@ static void set_sps(const HEVCSPS *sps, int sps_idx,
>
> *vksps_vui_header = (StdVideoH265HrdParameters) {
> .flags = (StdVideoH265HrdFlags) {
> - .nal_hrd_parameters_present_flag = sps->hdr.flags.nal_hrd_parameters_present_flag,
> - .vcl_hrd_parameters_present_flag = sps->hdr.flags.vcl_hrd_parameters_present_flag,
> - .sub_pic_hrd_params_present_flag = sps->hdr.flags.sub_pic_hrd_params_present_flag,
> - .sub_pic_cpb_params_in_pic_timing_sei_flag = sps->hdr.flags.sub_pic_cpb_params_in_pic_timing_sei_flag,
> + .nal_hrd_parameters_present_flag = sps->hdr.nal_hrd_parameters_present_flag,
> + .vcl_hrd_parameters_present_flag = sps->hdr.vcl_hrd_parameters_present_flag,
> + .sub_pic_hrd_params_present_flag = sps->hdr.sub_pic_hrd_params_present_flag,
> + .sub_pic_cpb_params_in_pic_timing_sei_flag = sps->hdr.sub_pic_cpb_params_in_pic_timing_sei_flag,
> .fixed_pic_rate_general_flag = sps->hdr.flags.fixed_pic_rate_general_flag,
> .fixed_pic_rate_within_cvs_flag = sps->hdr.flags.fixed_pic_rate_within_cvs_flag,
> .low_delay_hrd_flag = sps->hdr.flags.low_delay_hrd_flag,
> @@ -567,10 +567,10 @@ static void set_vps(const HEVCVPS *vps,
>
> sls_hdr[i] = (StdVideoH265HrdParameters) {
> .flags = (StdVideoH265HrdFlags) {
> - .nal_hrd_parameters_present_flag = src->flags.nal_hrd_parameters_present_flag,
> - .vcl_hrd_parameters_present_flag = src->flags.vcl_hrd_parameters_present_flag,
> - .sub_pic_hrd_params_present_flag = src->flags.sub_pic_hrd_params_present_flag,
> - .sub_pic_cpb_params_in_pic_timing_sei_flag = src->flags.sub_pic_cpb_params_in_pic_timing_sei_flag,
> + .nal_hrd_parameters_present_flag = src->nal_hrd_parameters_present_flag,
> + .vcl_hrd_parameters_present_flag = src->vcl_hrd_parameters_present_flag,
> + .sub_pic_hrd_params_present_flag = src->sub_pic_hrd_params_present_flag,
> + .sub_pic_cpb_params_in_pic_timing_sei_flag = src->sub_pic_cpb_params_in_pic_timing_sei_flag,
> .fixed_pic_rate_general_flag = src->flags.fixed_pic_rate_general_flag,
> .fixed_pic_rate_within_cvs_flag = src->flags.fixed_pic_rate_within_cvs_flag,
> .low_delay_hrd_flag = src->flags.low_delay_hrd_flag,
>
LGTM
Thanks
_______________________________________________
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".
prev parent reply other threads:[~2024-03-21 17:14 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-03-20 23:17 James Almer
2024-03-20 23:17 ` [FFmpeg-devel] [PATCH 2/2] avcodec/hevc_ps: use bitfields to slightly reduce the size of some structs James Almer
2024-03-20 23:43 ` James Almer
2024-03-21 17:14 ` Lynne [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=NtX-Oyf--3-9@lynne.ee \
--to=dev@lynne.ee \
--cc=ffmpeg-devel@ffmpeg.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Git Inbox Mirror of the ffmpeg-devel mailing list - see https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
This inbox may be cloned and mirrored by anyone:
git clone --mirror https://master.gitmailbox.com/ffmpegdev/0 ffmpegdev/git/0.git
# If you have public-inbox 1.1+ installed, you may
# initialize and index your mirror using the following commands:
public-inbox-init -V2 ffmpegdev ffmpegdev/ https://master.gitmailbox.com/ffmpegdev \
ffmpegdev@gitmailbox.com
public-inbox-index ffmpegdev
Example config snippet for mirrors.
AGPL code for this site: git clone https://public-inbox.org/public-inbox.git