LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: John Cox <jc@kynesim.co.uk>
To: Benjamin Gaignard <benjamin.gaignard@collabora.com>
Cc: mchehab@kernel.org, hverkuil-cisco@xs4all.nl,
	nicolas@ndufresne.ca, linux-media@vger.kernel.org,
	linux-kernel@vger.kernel.org, kernel@collabora.com,
	Benjamin Gaignard <benjamin.gaignard@collabora.com>
Subject: Re: [PATCH] media: hevc: fix pictures lists type
Date: Mon, 23 Aug 2021 10:50:51 +0100	[thread overview]
Message-ID: <02r6ig176o0lqc52nm8rhta7cn5bfn04in@4ax.com> (raw)
In-Reply-To: <20210823082949.237716-1-benjamin.gaignard@collabora.com>

>The lists embedded Picture Order Count values which are s32 so their type
>most be s32 and not u8.

I'm not convinced that you can't calculate all of those lists from the
info already contained in the DPB array so this is probably redundant
info though I grant that having the list pre-calced might make your life
easier, and the userland side will have calculated the lists to
calculate other required things so it isn't much extra work for it.

Even if you do need the lists wouldn't it be a better idea to have them
as indices into the DPB (you can't have a frame in any of those lists
that isn't in the DPB) which already contains POCs then it will still
fit into u8 and be smaller?

Full disclosure: Pi decode doesn't use this info at all so I'm only
arguing from a theoretical point of view - I think it is only relevant
if your h/w is parsing the reference list setups.

Regards

John Cox

>Reported-by: John Cox <jc@kynesim.co.uk>
>Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
>---
> Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst | 6 +++---
> include/media/hevc-ctrls.h                                | 6 +++---
> 2 files changed, 6 insertions(+), 6 deletions(-)
>
>diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
>index 976d34445a24..db9859ddc8b2 100644
>--- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
>+++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
>@@ -3323,15 +3323,15 @@ enum v4l2_mpeg_video_hevc_size_of_length_field -
>     * - __u8
>       - ``num_poc_lt_curr``
>       - The number of reference pictures in the long-term set.
>-    * - __u8
>+    * - __s32
>       - ``poc_st_curr_before[V4L2_HEVC_DPB_ENTRIES_NUM_MAX]``
>       - PocStCurrBefore as described in section 8.3.2 "Decoding process for reference
>         picture set.
>-    * - __u8
>+    * - __s32
>       - ``poc_st_curr_after[V4L2_HEVC_DPB_ENTRIES_NUM_MAX]``
>       - PocStCurrAfter as described in section 8.3.2 "Decoding process for reference
>         picture set.
>-    * - __u8
>+    * - __s32
>       - ``poc_lt_curr[V4L2_HEVC_DPB_ENTRIES_NUM_MAX]``
>       - PocLtCurr as described in section 8.3.2 "Decoding process for reference
>         picture set.
>diff --git a/include/media/hevc-ctrls.h b/include/media/hevc-ctrls.h
>index 781371bff2ad..04cd62e77f25 100644
>--- a/include/media/hevc-ctrls.h
>+++ b/include/media/hevc-ctrls.h
>@@ -219,9 +219,9 @@ struct v4l2_ctrl_hevc_decode_params {
> 	__u8	num_poc_st_curr_before;
> 	__u8	num_poc_st_curr_after;
> 	__u8	num_poc_lt_curr;
>-	__u8	poc_st_curr_before[V4L2_HEVC_DPB_ENTRIES_NUM_MAX];
>-	__u8	poc_st_curr_after[V4L2_HEVC_DPB_ENTRIES_NUM_MAX];
>-	__u8	poc_lt_curr[V4L2_HEVC_DPB_ENTRIES_NUM_MAX];
>+	__s32	poc_st_curr_before[V4L2_HEVC_DPB_ENTRIES_NUM_MAX];
>+	__s32	poc_st_curr_after[V4L2_HEVC_DPB_ENTRIES_NUM_MAX];
>+	__s32	poc_lt_curr[V4L2_HEVC_DPB_ENTRIES_NUM_MAX];
> 	__u64	flags;
> };
> 

  reply	other threads:[~2021-08-23  9:50 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-23  8:29 Benjamin Gaignard
2021-08-23  9:50 ` John Cox [this message]
2021-08-23 11:17   ` Benjamin Gaignard
2021-08-23 11:35     ` John Cox
2021-08-26 16:09       ` Nicolas Dufresne
2021-08-27  8:55         ` Benjamin Gaignard
2021-08-27 10:10           ` John Cox
2021-08-27 11:35             ` Benjamin Gaignard
2021-08-27 12:36               ` John Cox
2021-08-27 12:40                 ` Ezequiel Garcia
2021-08-27 15:26                   ` Benjamin Gaignard
2021-08-27 18:46                     ` Ezequiel Garcia

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=02r6ig176o0lqc52nm8rhta7cn5bfn04in@4ax.com \
    --to=jc@kynesim.co.uk \
    --cc=benjamin.gaignard@collabora.com \
    --cc=hverkuil-cisco@xs4all.nl \
    --cc=kernel@collabora.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-media@vger.kernel.org \
    --cc=mchehab@kernel.org \
    --cc=nicolas@ndufresne.ca \
    --subject='Re: [PATCH] media: hevc: fix pictures lists type' \
    /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

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).