From: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
To: Boris Brezillon <boris.brezillon@collabora.com>
Cc: Nicolas Dufresne <nicolas@ndufresne.ca>,
Ezequiel Garcia <ezequiel@collabora.com>,
linux-media@vger.kernel.org, devicetree@vger.kernel.org,
linux-rockchip@lists.infradead.org, linux-kernel@vger.kernel.org,
Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
Rob Herring <robh+dt@kernel.org>,
Tomasz Figa <tfiga@chromium.org>,
kernel@collabora.com,
Paul Kocialkowski <paul.kocialkowski@bootlin.com>,
Jonas Karlman <jonas@kwiboo.se>, Heiko Stuebner <heiko@sntech.de>,
Sakari Ailus <sakari.ailus@iki.fi>,
Hans Verkuil <hverkuil@xs4all.nl>
Subject: Re: [PATCH v6 2/6] media: v4l2-core: Add helpers to build the H264 P/B0/B1 reflists
Date: Thu, 5 Mar 2020 22:37:50 +0100 [thread overview]
Message-ID: <20200305223750.14514063@coco.lan> (raw)
In-Reply-To: <20200305211535.2e9a6673@collabora.com>
Em Thu, 5 Mar 2020 21:15:35 +0100
Boris Brezillon <boris.brezillon@collabora.com> escreveu:
> On Thu, 05 Mar 2020 14:42:34 -0500
> Nicolas Dufresne <nicolas@ndufresne.ca> wrote:
>
> > Le lundi 02 mars 2020 à 15:44 +0100, Boris Brezillon a écrit :
> > > On Mon, 2 Mar 2020 14:24:33 +0100
> > > Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote:
> > >
> > > > Em Thu, 20 Feb 2020 13:30:12 -0300
> > > > Ezequiel Garcia <ezequiel@collabora.com> escreveu:
> > > >
> > > > > From: Boris Brezillon <boris.brezillon@collabora.com>
> > > > >
> > > > > Building those list is a standard procedure described in section
> > > > > '8.2.4 Decoding process for reference picture lists construction' of
> > > > > the H264 specification.
> > > > >
> > > > > We already have 2 drivers needing the same logic (hantro and rkvdec) and
> > > > > I suspect we will soon have more.
> > > > >
> > > > > Let's provide generic helpers to create those lists.
> > > > >
> > > > > Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> > > > > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> > > > > ---
> > > > > drivers/media/v4l2-core/Kconfig | 4 +
> > > > > drivers/media/v4l2-core/Makefile | 1 +
> > > > > drivers/media/v4l2-core/v4l2-h264.c | 258 ++++++++++++++++++++++++++++
> > > > > include/media/v4l2-h264.h | 85 +++++++++
> > > > > 4 files changed, 348 insertions(+)
> > > > > create mode 100644 drivers/media/v4l2-core/v4l2-h264.c
> > > > > create mode 100644 include/media/v4l2-h264.h
> > > > >
> > > > > diff --git a/drivers/media/v4l2-core/Kconfig b/drivers/media/v4l2-
> > > > > core/Kconfig
> > > > > index 39e3fb30ba0b..8a4ccfbca8cf 100644
> > > > > --- a/drivers/media/v4l2-core/Kconfig
> > > > > +++ b/drivers/media/v4l2-core/Kconfig
> > > > > @@ -45,6 +45,10 @@ config VIDEO_PCI_SKELETON
> > > > > config VIDEO_TUNER
> > > > > tristate
> > > > >
> > > > > +# Used by drivers that need v4l2-h264.ko
> > > > > +config V4L2_H264
> > > > > + tristate
> > > > > +
> > > > > # Used by drivers that need v4l2-mem2mem.ko
> > > > > config V4L2_MEM2MEM_DEV
> > > > > tristate
> > > > > diff --git a/drivers/media/v4l2-core/Makefile b/drivers/media/v4l2-
> > > > > core/Makefile
> > > > > index 786bd1ec4d1b..c5c53e0941ad 100644
> > > > > --- a/drivers/media/v4l2-core/Makefile
> > > > > +++ b/drivers/media/v4l2-core/Makefile
> > > > > @@ -21,6 +21,7 @@ obj-$(CONFIG_VIDEO_V4L2) += v4l2-dv-timings.o
> > > > > obj-$(CONFIG_VIDEO_TUNER) += tuner.o
> > > > >
> > > > > obj-$(CONFIG_V4L2_MEM2MEM_DEV) += v4l2-mem2mem.o
> > > > > +obj-$(CONFIG_V4L2_H264) += v4l2-h264.o
> > > > >
> > > > > obj-$(CONFIG_V4L2_FLASH_LED_CLASS) += v4l2-flash-led-class.o
> > > > >
> > > > > diff --git a/drivers/media/v4l2-core/v4l2-h264.c b/drivers/media/v4l2-
> > > > > core/v4l2-h264.c
> > > > > new file mode 100644
> > > > > index 000000000000..4f68c27ec7fd
> > > > > --- /dev/null
> > > > > +++ b/drivers/media/v4l2-core/v4l2-h264.c
> > > > > @@ -0,0 +1,258 @@
> > > > > +// SPDX-License-Identifier: GPL-2.0
> > > > > +/*
> > > > > + * V4L2 H264 helpers.
> > > > > + *
> > > > > + * Copyright (C) 2019 Collabora, Ltd.
> > > > > + *
> > > > > + * Author: Boris Brezillon <boris.brezillon@collabora.com>
> > > > > + */
> > > > > +
> > > > > +#include <linux/module.h>
> > > > > +#include <linux/sort.h>
> > > > > +
> > > > > +#include <media/v4l2-h264.h>
> > > > > +
> > > > > +/**
> > > > > + * v4l2_h264_init_reflist_builder() - Initialize a P/B0/B1 reference list
> > > > > + * builder
> > > > > + *
> > > > > + * @b: the builder context to initialize
> > > > > + * @dec_params: decode parameters control
> > > > > + * @slice_params: first slice parameters control
> > > > > + * @sps: SPS control
> > > > > + * @dpb: DPB to use when creating the reference list
> > > > > + */
> > > > > +void
> > > > > +v4l2_h264_init_reflist_builder(struct v4l2_h264_reflist_builder *b,
> > > > > + const struct v4l2_ctrl_h264_decode_params *dec_params,
> > > > > + const struct v4l2_ctrl_h264_slice_params *slice_params,
> > > > > + const struct v4l2_ctrl_h264_sps *sps,
> > > > > + const struct v4l2_h264_dpb_entry *dpb)
> > > >
> > > > The prototype here is not nice...
> > > >
> > > > > +{
> > > > > + int cur_frame_num, max_frame_num;
> > > > > + unsigned int i;
> > > > > +
> > > > > + max_frame_num = 1 << (sps->log2_max_frame_num_minus4 + 4);
> > > > > + cur_frame_num = slice_params->frame_num;
> > > > > +
> > > > > + memset(b, 0, sizeof(*b));
> > > > > + if (!(slice_params->flags & V4L2_H264_SLICE_FLAG_FIELD_PIC))
> > > > > + b->cur_pic_order_count = min(dec_params->bottom_field_order_cnt,
> > > > > + dec_params->top_field_order_cnt);
> > > > > + else if (slice_params->flags & V4L2_H264_SLICE_FLAG_BOTTOM_FIELD)
> > > > > + b->cur_pic_order_count = dec_params->bottom_field_order_cnt;
> > > > > + else
> > > > > + b->cur_pic_order_count = dec_params->top_field_order_cnt;
> > > > > +
> > > > > + for (i = 0; i < 16; i++) {
> > > > > + u32 pic_order_count;
> > > > > +
> > > > > + if (!(dpb[i].flags & V4L2_H264_DPB_ENTRY_FLAG_ACTIVE))
> > > > > + continue;
> > > > > +
> > > > > + b->refs[i].pic_num = dpb[i].pic_num;
> > > >
> > > > ... as you're expecting a fixed number of elements at DPB array, and using
> > > > a magic number (16) inside the for loop.
> > >
> > > I used to have a '#define V4L2_H264_NUM_DPB_ENTRIES 16' but have been
> > > told that this is an arbitrary limitation (the spec does not explicitly
> > > limit the DPB size, even if all the HW we've seen seem to limit it to
> > > 16). Maybe we can pass the DPB array size as an argument so it stays
> > > HW-specific.
> >
> > it's formalized in A.31 h), to quote it:
> >
> > max_dec_frame_buffering <= MaxDpbFrames, where MaxDpbFrames is equal to
> >
> > Min( MaxDpbMbs / ( PicWidthInMbs * FrameHeightInMbs ), 16 )
> >
> > So a DPB larger then this is not an H.24 DPB.
>
> My bad, I mixed "max number of slices per frame" and "max number of
> entries in the DPB". The first one I couldn't find a clear answer to,
> while the second one has been 16 from the start. Sorry for the
> confusion.
Well, newer versions of the spec might change it. In any case, I guess
a define should work properly. If the spec is ever updated to accept
a bigger number, a simple change at the define would be enough to
solve.
Thanks,
Mauro
next prev parent reply other threads:[~2020-03-05 21:38 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-02-20 16:30 [PATCH v6 0/6] media: rockchip: Add the rkvdec driver Ezequiel Garcia
2020-02-20 16:30 ` [PATCH v6 1/6] media: uapi: h264: Add DPB entry field reference flags Ezequiel Garcia
2020-02-20 16:30 ` [PATCH v6 2/6] media: v4l2-core: Add helpers to build the H264 P/B0/B1 reflists Ezequiel Garcia
2020-03-02 13:24 ` Mauro Carvalho Chehab
2020-03-02 14:44 ` Boris Brezillon
2020-03-02 15:21 ` Mauro Carvalho Chehab
2020-03-05 19:42 ` Nicolas Dufresne
2020-03-05 20:15 ` Boris Brezillon
2020-03-05 21:37 ` Mauro Carvalho Chehab [this message]
2020-02-20 16:30 ` [PATCH v6 3/6] media: hantro: h264: Use the generic H264 reflist builder Ezequiel Garcia
2020-02-20 16:30 ` [PATCH v6 4/6] media: dt-bindings: rockchip: Document RK3399 Video Decoder bindings Ezequiel Garcia
2020-02-20 16:30 ` [PATCH v6 5/6] media: rkvdec: Add the rkvdec driver Ezequiel Garcia
2020-03-02 13:57 ` Mauro Carvalho Chehab
2020-03-02 14:30 ` Boris Brezillon
2020-03-02 14:53 ` Mauro Carvalho Chehab
2020-03-02 14:35 ` Boris Brezillon
2020-03-02 14:39 ` Mauro Carvalho Chehab
2020-02-20 16:30 ` [PATCH v6 6/6] arm64: dts: rockchip: rk3399: Define the rockchip Video Decoder node Ezequiel Garcia
2020-02-26 12:24 ` Johan Jonker
2020-02-26 13:21 ` Heiko Stuebner
2020-02-26 17:22 ` Ezequiel Garcia
2020-03-01 0:14 ` Heiko Stuebner
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=20200305223750.14514063@coco.lan \
--to=mchehab+huawei@kernel.org \
--cc=boris.brezillon@collabora.com \
--cc=devicetree@vger.kernel.org \
--cc=ezequiel@collabora.com \
--cc=heiko@sntech.de \
--cc=hverkuil@xs4all.nl \
--cc=jonas@kwiboo.se \
--cc=kernel@collabora.com \
--cc=laurent.pinchart@ideasonboard.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-media@vger.kernel.org \
--cc=linux-rockchip@lists.infradead.org \
--cc=nicolas@ndufresne.ca \
--cc=paul.kocialkowski@bootlin.com \
--cc=robh+dt@kernel.org \
--cc=sakari.ailus@iki.fi \
--cc=tfiga@chromium.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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).