From: Nicolas Dufresne <nicolas.dufresne@collabora.com> To: Tomasz Figa <tfiga@chromium.org>, Paul Kocialkowski <paul.kocialkowski@bootlin.com> Cc: Maxime Ripard <maxime.ripard@bootlin.com>, Ezequiel Garcia <ezequiel@collabora.com>, Pawel Osciak <posciak@chromium.org>, Hans Verkuil <hans.verkuil@cisco.com>, Alexandre Courbot <acourbot@chromium.org>, Sakari Ailus <sakari.ailus@linux.intel.com>, Laurent Pinchart <laurent.pinchart@ideasonboard.com>, Chen-Yu Tsai <wens@csie.org>, Linux Kernel Mailing List <linux-kernel@vger.kernel.org>, "list@263.net:IOMMU DRIVERS <iommu@lists.linux-foundation.org>, Joerg " "Roedel <joro@8bytes.org>," <linux-arm-kernel@lists.infradead.org>, Linux Media Mailing List <linux-media@vger.kernel.org>, jenskuske@gmail.com, linux-sunxi@googlegroups.com, thomas.petazzoni@bootlin.com, groeck@chromium.org Subject: Re: [PATCH 1/9] CHROMIUM: v4l: Add H264 low-level decoder API compound controls. Date: Wed, 22 Aug 2018 10:03:05 -0400 [thread overview] Message-ID: <2550f0d2fd955922c8d6508573ac78487315bf25.camel@collabora.com> (raw) In-Reply-To: <CAAFQd5AybueM-rhy8bMbRHHZmwBP08-cWB11NadcUQAb6XJ1SA@mail.gmail.com> [-- Attachment #1: Type: text/plain, Size: 7017 bytes --] Le mercredi 22 août 2018 à 22:24 +0900, Tomasz Figa a écrit : > On Wed, Aug 22, 2018 at 10:03 PM Paul Kocialkowski > <paul.kocialkowski@bootlin.com> wrote: > > > > Hi, > > > > On Wed, 2018-08-22 at 18:54 +0900, Tomasz Figa wrote: > > > On Wed, Aug 22, 2018 at 6:16 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote: > > > > > > > > Hi, > > > > > > > > On Tue, Aug 21, 2018 at 01:58:38PM -0300, Ezequiel Garcia wrote: > > > > > On Wed, 2018-06-13 at 16:07 +0200, Maxime Ripard wrote: > > > > > > From: Pawel Osciak <posciak@chromium.org> > > > > > > > > > > > > Signed-off-by: Pawel Osciak <posciak@chromium.org> > > > > > > Reviewed-by: Wu-cheng Li <wuchengli@chromium.org> > > > > > > Tested-by: Tomasz Figa <tfiga@chromium.org> > > > > > > [rebase44(groeck): include linux/types.h in v4l2-controls.h] > > > > > > Signed-off-by: Guenter Roeck <groeck@chromium.org> > > > > > > Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com> > > > > > > --- > > > > > > > > > > > > > > > > [..] > > > > > > diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h > > > > > > index 242a6bfa1440..4b4a1b25a0db 100644 > > > > > > --- a/include/uapi/linux/videodev2.h > > > > > > +++ b/include/uapi/linux/videodev2.h > > > > > > @@ -626,6 +626,7 @@ struct v4l2_pix_format { > > > > > > #define V4L2_PIX_FMT_H264 v4l2_fourcc('H', '2', '6', '4') /* H264 with start codes */ > > > > > > #define V4L2_PIX_FMT_H264_NO_SC v4l2_fourcc('A', 'V', 'C', '1') /* H264 without start codes */ > > > > > > #define V4L2_PIX_FMT_H264_MVC v4l2_fourcc('M', '2', '6', '4') /* H264 MVC */ > > > > > > +#define V4L2_PIX_FMT_H264_SLICE v4l2_fourcc('S', '2', '6', '4') /* H264 parsed slices */ > > > > > > > > > > As pointed out by Tomasz, the Rockchip VPU driver expects start codes [1], so the userspace > > > > > should be aware of it. Perhaps we could document this pixel format better as: > > > > > > > > > > #define V4L2_PIX_FMT_H264_SLICE v4l2_fourcc('S', '2', '6', '4') /* H264 parsed slices with start codes */ > > > > > > > > I'm not sure this is something we want to do at that point. libva > > > > doesn't give the start code, so this is only going to make the life of > > > > the sane controllers more difficult. And if you need to have the start > > > > code and parse it, then you're not so stateless anymore. > > > > > > I might not remember correctly, but Rockchip decoder does some slice > > > parsing on its own (despite not doing any higher level parsing). > > > Probably that's why it needs those start codes. > > > > The VPU found on Allwinner platforms also provides a mechanism to parse > > the bitstream data via a dedicated interface through the VPU registers. > > It is used in libvdpau-sunxi but not in our driver, because we don't > > want to be doing bitstream parsing in the kernel. > > > > It would be good to know if this is just a feature of the Rockchip VPU > > hardware that can be skipped (like on Allwinner) or if it's a hard > > requirement in its decoding pipeline. > > It's a hard requirement for its decoding pipeline, but... > > > Also, maybe it only concerns the > > slice header? It is already part of the slice data (provided by VAAPI) > > for H.264/H.265 and an offset is provided to the beginning of the coded > > video data. > > Yes, it seems to be only the slice header. > > > > > > I wonder if libva is the best reference here. It's been designed > > > almost entirely by Intel for Intel video hardware. We want something > > > that could work with a wide range of devices and avoid something like > > > a need to create a semi-stateless API few months later. In fact, > > > hardware from another vendor, we're working with, also does parsing of > > > slice headers internally. Moreover, we have some weird > > > kind-of-stateful decoders, which cannot fully deal with bitstream on > > > its own, e.g. cannot parse formats, cannot handle resolution changes, > > > need H264 bitstream NALUs split into separate buffers, etc. > > > > > > As I suggested some time ago, having the full bitstream in the buffer, > > > with offsets of particular units included in respective controls, > > > would be the most scalable thing. If really needed, we could add flags > > > telling the driver that particular units are present, so one's > > > implementation of libva could put only raw slice data in the buffers. > > > But perhaps it's libva which needs some amendment? > > > > If the raw bitstream is needed, I think it would make more sense to use > > the already-existing formats for stateful VPUs along with the controls > > for stateless ones instead of having the full bitstream in the > > V4L2_PIX_FMT_*_SLICE formats. > > It may indeed make sense to separate this based on pixel format. > However, how do we tell the client that it needs to provide those > controls? Current concept was based entirely on pixel format, so I > guess that would mean creating something like > V4L2_PIX_FMT_*_NOT_REALLY_SLICE (_PARSED, _STATELESS?). Might be okay, > though... > > > > > I would also be tempted to say that reconstructing the needed parts of > > the bitstream in-driver for these half-way VPUs would be a better > > approach than blurrying the line between how (and what) data should be > > passed for stateful and stateless VPUs at the API level. Stateless > > should only cover what's in the slice NAL unit RBSP, which excludes the > > start code detection bytes. It is no longer parsed data otherwise. > > I'm not sure where such decision comes from. In particular, Chromium, > from which this code originates, includes start codes in > V4L2_PIX_FMT_H264_SLICE. As I mentioned earlier, we can't design this > API based only on 1 type of hardware semantics. The stateless API > should cover any kind of codec that needs user space assistance in > processing the stream, which in practice would be almost everything > for which stateful API doesn't work. Well, be aware that in most of today's use case, Chromium bitstream handling is suboptimal since the startcode are crafted by Chromium. Most of today's use case uses ISOMP4 container, which does not hold start codes but AVC headers. If we step back a little, the start codes are designed for randomly accessing bytes. It's a mark made to be able to randomly scan and find the start of a NAL. In a well framed stream, they are useless. Then AVC header is designed to announce the size of the following block, if we pass only one NAL by buffer, this is not very useful either. Both seems trivial to reconstruct if the HW requires it. > > That said, since pixel format essentially specifies the buffer > contents, having such cases differentiated based on the pixel format > doesn't sound insane. As of today, that's what the pixel formats tries to do with these _NO_SC postfix (the current naming is generally ambiguous though). > > Best regards, > Tomasz [-- Attachment #2: This is a digitally signed message part --] [-- Type: application/pgp-signature, Size: 195 bytes --]
WARNING: multiple messages have this Message-ID (diff)
From: nicolas.dufresne@collabora.com (Nicolas Dufresne) To: linux-arm-kernel@lists.infradead.org Subject: [PATCH 1/9] CHROMIUM: v4l: Add H264 low-level decoder API compound controls. Date: Wed, 22 Aug 2018 10:03:05 -0400 [thread overview] Message-ID: <2550f0d2fd955922c8d6508573ac78487315bf25.camel@collabora.com> (raw) In-Reply-To: <CAAFQd5AybueM-rhy8bMbRHHZmwBP08-cWB11NadcUQAb6XJ1SA@mail.gmail.com> Le mercredi 22 ao?t 2018 ? 22:24 +0900, Tomasz Figa a ?crit : > On Wed, Aug 22, 2018 at 10:03 PM Paul Kocialkowski > <paul.kocialkowski@bootlin.com> wrote: > > > > Hi, > > > > On Wed, 2018-08-22 at 18:54 +0900, Tomasz Figa wrote: > > > On Wed, Aug 22, 2018 at 6:16 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote: > > > > > > > > Hi, > > > > > > > > On Tue, Aug 21, 2018 at 01:58:38PM -0300, Ezequiel Garcia wrote: > > > > > On Wed, 2018-06-13 at 16:07 +0200, Maxime Ripard wrote: > > > > > > From: Pawel Osciak <posciak@chromium.org> > > > > > > > > > > > > Signed-off-by: Pawel Osciak <posciak@chromium.org> > > > > > > Reviewed-by: Wu-cheng Li <wuchengli@chromium.org> > > > > > > Tested-by: Tomasz Figa <tfiga@chromium.org> > > > > > > [rebase44(groeck): include linux/types.h in v4l2-controls.h] > > > > > > Signed-off-by: Guenter Roeck <groeck@chromium.org> > > > > > > Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com> > > > > > > --- > > > > > > > > > > > > > > > > [..] > > > > > > diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h > > > > > > index 242a6bfa1440..4b4a1b25a0db 100644 > > > > > > --- a/include/uapi/linux/videodev2.h > > > > > > +++ b/include/uapi/linux/videodev2.h > > > > > > @@ -626,6 +626,7 @@ struct v4l2_pix_format { > > > > > > #define V4L2_PIX_FMT_H264 v4l2_fourcc('H', '2', '6', '4') /* H264 with start codes */ > > > > > > #define V4L2_PIX_FMT_H264_NO_SC v4l2_fourcc('A', 'V', 'C', '1') /* H264 without start codes */ > > > > > > #define V4L2_PIX_FMT_H264_MVC v4l2_fourcc('M', '2', '6', '4') /* H264 MVC */ > > > > > > +#define V4L2_PIX_FMT_H264_SLICE v4l2_fourcc('S', '2', '6', '4') /* H264 parsed slices */ > > > > > > > > > > As pointed out by Tomasz, the Rockchip VPU driver expects start codes [1], so the userspace > > > > > should be aware of it. Perhaps we could document this pixel format better as: > > > > > > > > > > #define V4L2_PIX_FMT_H264_SLICE v4l2_fourcc('S', '2', '6', '4') /* H264 parsed slices with start codes */ > > > > > > > > I'm not sure this is something we want to do at that point. libva > > > > doesn't give the start code, so this is only going to make the life of > > > > the sane controllers more difficult. And if you need to have the start > > > > code and parse it, then you're not so stateless anymore. > > > > > > I might not remember correctly, but Rockchip decoder does some slice > > > parsing on its own (despite not doing any higher level parsing). > > > Probably that's why it needs those start codes. > > > > The VPU found on Allwinner platforms also provides a mechanism to parse > > the bitstream data via a dedicated interface through the VPU registers. > > It is used in libvdpau-sunxi but not in our driver, because we don't > > want to be doing bitstream parsing in the kernel. > > > > It would be good to know if this is just a feature of the Rockchip VPU > > hardware that can be skipped (like on Allwinner) or if it's a hard > > requirement in its decoding pipeline. > > It's a hard requirement for its decoding pipeline, but... > > > Also, maybe it only concerns the > > slice header? It is already part of the slice data (provided by VAAPI) > > for H.264/H.265 and an offset is provided to the beginning of the coded > > video data. > > Yes, it seems to be only the slice header. > > > > > > I wonder if libva is the best reference here. It's been designed > > > almost entirely by Intel for Intel video hardware. We want something > > > that could work with a wide range of devices and avoid something like > > > a need to create a semi-stateless API few months later. In fact, > > > hardware from another vendor, we're working with, also does parsing of > > > slice headers internally. Moreover, we have some weird > > > kind-of-stateful decoders, which cannot fully deal with bitstream on > > > its own, e.g. cannot parse formats, cannot handle resolution changes, > > > need H264 bitstream NALUs split into separate buffers, etc. > > > > > > As I suggested some time ago, having the full bitstream in the buffer, > > > with offsets of particular units included in respective controls, > > > would be the most scalable thing. If really needed, we could add flags > > > telling the driver that particular units are present, so one's > > > implementation of libva could put only raw slice data in the buffers. > > > But perhaps it's libva which needs some amendment? > > > > If the raw bitstream is needed, I think it would make more sense to use > > the already-existing formats for stateful VPUs along with the controls > > for stateless ones instead of having the full bitstream in the > > V4L2_PIX_FMT_*_SLICE formats. > > It may indeed make sense to separate this based on pixel format. > However, how do we tell the client that it needs to provide those > controls? Current concept was based entirely on pixel format, so I > guess that would mean creating something like > V4L2_PIX_FMT_*_NOT_REALLY_SLICE (_PARSED, _STATELESS?). Might be okay, > though... > > > > > I would also be tempted to say that reconstructing the needed parts of > > the bitstream in-driver for these half-way VPUs would be a better > > approach than blurrying the line between how (and what) data should be > > passed for stateful and stateless VPUs at the API level. Stateless > > should only cover what's in the slice NAL unit RBSP, which excludes the > > start code detection bytes. It is no longer parsed data otherwise. > > I'm not sure where such decision comes from. In particular, Chromium, > from which this code originates, includes start codes in > V4L2_PIX_FMT_H264_SLICE. As I mentioned earlier, we can't design this > API based only on 1 type of hardware semantics. The stateless API > should cover any kind of codec that needs user space assistance in > processing the stream, which in practice would be almost everything > for which stateful API doesn't work. Well, be aware that in most of today's use case, Chromium bitstream handling is suboptimal since the startcode are crafted by Chromium. Most of today's use case uses ISOMP4 container, which does not hold start codes but AVC headers. If we step back a little, the start codes are designed for randomly accessing bytes. It's a mark made to be able to randomly scan and find the start of a NAL. In a well framed stream, they are useless. Then AVC header is designed to announce the size of the following block, if we pass only one NAL by buffer, this is not very useful either. Both seems trivial to reconstruct if the HW requires it. > > That said, since pixel format essentially specifies the buffer > contents, having such cases differentiated based on the pixel format > doesn't sound insane. As of today, that's what the pixel formats tries to do with these _NO_SC postfix (the current naming is generally ambiguous though). > > Best regards, > Tomasz -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 195 bytes Desc: This is a digitally signed message part URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20180822/7791baf8/attachment.sig>
next prev parent reply other threads:[~2018-08-22 14:03 UTC|newest] Thread overview: 102+ messages / expand[flat|nested] mbox.gz Atom feed top 2018-06-13 14:07 [PATCH 0/9] media: cedrus: Add H264 decoding support Maxime Ripard 2018-06-13 14:07 ` Maxime Ripard 2018-06-13 14:07 ` [PATCH 1/9] CHROMIUM: v4l: Add H264 low-level decoder API compound controls Maxime Ripard 2018-06-13 14:07 ` Maxime Ripard 2018-06-15 11:59 ` Hans Verkuil 2018-06-15 11:59 ` Hans Verkuil 2018-06-15 13:01 ` Guenter Roeck 2018-06-15 13:01 ` Guenter Roeck 2018-07-12 16:38 ` Maxime Ripard 2018-07-12 16:38 ` Maxime Ripard 2018-07-12 16:47 ` Andrew Lunn 2018-07-12 16:47 ` Andrew Lunn 2018-06-21 8:58 ` Paul Kocialkowski 2018-06-21 8:58 ` Paul Kocialkowski 2018-08-21 16:58 ` Ezequiel Garcia 2018-08-21 16:58 ` Ezequiel Garcia 2018-08-21 17:07 ` Nicolas Dufresne 2018-08-21 17:07 ` Nicolas Dufresne 2018-08-22 13:07 ` Paul Kocialkowski 2018-08-22 13:07 ` Paul Kocialkowski 2018-08-22 13:38 ` Tomasz Figa 2018-08-22 13:38 ` Tomasz Figa 2018-08-22 13:52 ` Nicolas Dufresne 2018-08-22 13:52 ` Nicolas Dufresne 2018-08-22 14:45 ` Paul Kocialkowski 2018-08-22 14:45 ` Paul Kocialkowski 2018-08-28 8:11 ` Tomasz Figa 2018-08-28 8:11 ` Tomasz Figa 2018-09-07 7:54 ` Tomasz Figa 2018-09-07 7:54 ` Tomasz Figa 2018-08-22 9:15 ` Maxime Ripard 2018-08-22 9:15 ` Maxime Ripard 2018-08-22 9:54 ` Tomasz Figa 2018-08-22 9:54 ` Tomasz Figa 2018-08-22 13:03 ` Paul Kocialkowski 2018-08-22 13:03 ` Paul Kocialkowski 2018-08-22 13:24 ` Tomasz Figa 2018-08-22 13:24 ` Tomasz Figa 2018-08-22 14:03 ` Nicolas Dufresne [this message] 2018-08-22 14:03 ` Nicolas Dufresne 2018-08-22 14:30 ` Paul Kocialkowski 2018-08-22 14:30 ` Paul Kocialkowski 2018-06-13 14:07 ` [PATCH 2/9] media: cedrus: Add wrappers around container_of for our buffers Maxime Ripard 2018-06-13 14:07 ` Maxime Ripard 2018-06-21 9:03 ` Paul Kocialkowski 2018-06-21 9:03 ` Paul Kocialkowski 2018-06-13 14:07 ` [PATCH 3/9] media: cedrus: Add a macro to check for the validity of a control Maxime Ripard 2018-06-13 14:07 ` Maxime Ripard 2018-06-21 9:13 ` Paul Kocialkowski 2018-06-21 9:13 ` Paul Kocialkowski 2018-06-13 14:07 ` [PATCH 4/9] media: cedrus: make engine type more generic Maxime Ripard 2018-06-13 14:07 ` Maxime Ripard 2018-06-21 9:33 ` Paul Kocialkowski 2018-06-21 9:33 ` Paul Kocialkowski 2018-06-13 14:07 ` [PATCH 5/9] media: cedrus: Remove MPEG1 support Maxime Ripard 2018-06-13 14:07 ` Maxime Ripard 2018-06-13 14:07 ` [PATCH 6/9] media: cedrus: Add ops structure Maxime Ripard 2018-06-13 14:07 ` Maxime Ripard 2018-06-21 9:49 ` Paul Kocialkowski 2018-06-21 9:49 ` Paul Kocialkowski 2018-06-25 13:29 ` Maxime Ripard 2018-06-25 13:29 ` Maxime Ripard 2018-06-25 13:48 ` Paul Kocialkowski 2018-06-25 13:48 ` Paul Kocialkowski 2018-06-13 14:07 ` [PATCH 7/9] media: cedrus: Move IRQ maintainance to cedrus_dec_ops Maxime Ripard 2018-06-13 14:07 ` Maxime Ripard 2018-06-21 15:35 ` Paul Kocialkowski 2018-06-21 15:35 ` Paul Kocialkowski 2018-06-25 14:18 ` Paul Kocialkowski 2018-06-25 14:18 ` Paul Kocialkowski 2018-06-25 16:15 ` Maxime Ripard 2018-06-25 16:15 ` Maxime Ripard 2018-06-25 15:38 ` Paul Kocialkowski 2018-06-25 15:38 ` Paul Kocialkowski 2018-06-25 15:49 ` Paul Kocialkowski 2018-06-25 15:49 ` Paul Kocialkowski 2018-06-25 19:01 ` Maxime Ripard 2018-06-25 19:01 ` Maxime Ripard 2018-06-27 17:58 ` Maxime Ripard 2018-06-27 17:58 ` Maxime Ripard 2018-06-13 14:07 ` [PATCH 8/9] media: cedrus: Add start and stop decoder operations Maxime Ripard 2018-06-13 14:07 ` Maxime Ripard 2018-06-21 15:38 ` Paul Kocialkowski 2018-06-21 15:38 ` Paul Kocialkowski 2018-06-25 13:32 ` Maxime Ripard 2018-06-25 13:32 ` Maxime Ripard 2018-06-25 13:42 ` Paul Kocialkowski 2018-06-25 13:42 ` Paul Kocialkowski 2018-06-13 14:07 ` [PATCH 9/9] media: cedrus: Add H264 decoding support Maxime Ripard 2018-06-13 14:07 ` Maxime Ripard 2018-07-27 13:56 ` Paul Kocialkowski 2018-07-27 13:56 ` Paul Kocialkowski 2018-07-27 14:01 ` Chen-Yu Tsai 2018-07-27 14:01 ` Chen-Yu Tsai 2018-07-27 14:03 ` Paul Kocialkowski 2018-07-27 14:03 ` Paul Kocialkowski 2018-07-30 12:54 ` Paul Kocialkowski 2018-07-30 12:54 ` Paul Kocialkowski 2018-06-14 13:00 ` [PATCH 0/9] " Tomasz Figa 2018-06-14 13:00 ` Tomasz Figa 2018-06-14 16:37 ` Maxime Ripard 2018-06-14 16:37 ` Maxime Ripard
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=2550f0d2fd955922c8d6508573ac78487315bf25.camel@collabora.com \ --to=nicolas.dufresne@collabora.com \ --cc=acourbot@chromium.org \ --cc=ezequiel@collabora.com \ --cc=groeck@chromium.org \ --cc=hans.verkuil@cisco.com \ --cc=jenskuske@gmail.com \ --cc=laurent.pinchart@ideasonboard.com \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-media@vger.kernel.org \ --cc=linux-sunxi@googlegroups.com \ --cc=maxime.ripard@bootlin.com \ --cc=paul.kocialkowski@bootlin.com \ --cc=posciak@chromium.org \ --cc=sakari.ailus@linux.intel.com \ --cc=tfiga@chromium.org \ --cc=thomas.petazzoni@bootlin.com \ --cc=wens@csie.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: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.