Re: [PATCH v5 5/6] media: Add controls for JPEG quantization tables

[Paul Kocialkowski <contact-W9ppeneeCTY@public.gmane.org>]

[-- Attachment #1.1: Type: text/plain, Size: 4957 bytes --]

Hi,

Le mercredi 19 septembre 2018 à 13:28 +0900, Tomasz Figa a écrit :
> On Thu, Sep 13, 2018 at 9:15 PM Paul Kocialkowski <contact-W9ppeneeCTY@public.gmane.org> wrote:
> > Hi,
> > 
> > On Wed, 2018-09-05 at 19:00 -0300, Ezequiel Garcia wrote:
> > > From: Shunqian Zheng <zhengsq-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
> > > 
> > > Add V4L2_CID_JPEG_QUANTIZATION compound control to allow userspace
> > > configure the JPEG quantization tables.
> > > 
> > > Signed-off-by: Shunqian Zheng <zhengsq-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
> > > Signed-off-by: Ezequiel Garcia <ezequiel-ZGY8ohtN/8qB+jHODAdFcQ@public.gmane.org>
> > > ---
> > >  .../media/uapi/v4l/extended-controls.rst      | 31 +++++++++++++++++++
> > >  .../media/videodev2.h.rst.exceptions          |  1 +
> > >  drivers/media/v4l2-core/v4l2-ctrls.c          | 10 ++++++
> > >  include/uapi/linux/v4l2-controls.h            | 12 +++++++
> > >  include/uapi/linux/videodev2.h                |  1 +
> > >  5 files changed, 55 insertions(+)
> > > 
> > > diff --git a/Documentation/media/uapi/v4l/extended-controls.rst b/Documentation/media/uapi/v4l/extended-controls.rst
> > > index 9f7312bf3365..1335d27d30f3 100644
> > > --- a/Documentation/media/uapi/v4l/extended-controls.rst
> > > +++ b/Documentation/media/uapi/v4l/extended-controls.rst
> > > @@ -3354,7 +3354,38 @@ JPEG Control IDs
> > >      Specify which JPEG markers are included in compressed stream. This
> > >      control is valid only for encoders.
> > > 
> > > +.. _jpeg-quant-tables-control:
> > 
> > I just had a look at how the Allwinner VPU handles JPEG decoding and it
> > seems to require the following information (in addition to
> > quantization):
> 
> I assume the hardware doesn't have the ability to parse those from the
> stream and so they need to be parsed by user space and given to the
> driver?

That's correct, we are also dealing with a stateless decoder here. It's
actually the same hardware engine that's used for MPEG2 decoding, only
configured differently.

So we will need to introduce a pixfmt for compressed JPEG data without
headers, reuse JPEG controls that apply and perhaps introduce new ones
too if needed.

I am also wondering about how MJPEG support should fit into this. As
far as I understood, it shouldn't be very different from JPEG so we
might want to have common controls for both.

> > * Horizontal and vertical sampling factors for each Y/U/V component:
> > 
> > The number of components and sampling factors are coded separately in
> > the bitstream, but it's probably easier to use the already-existing
> > V4L2_CID_JPEG_CHROMA_SUBSAMPLING control for specifying that.
> > 
> > However, this is potentially very much related to the destination
> > format. If we decide that this format should match the format resulting
> > from decompression, we don't need to specify it through an external
> > control. On the other hand, it's possible that the VPU has format
> > conversion block integrated in its pipeline so it would also make sense
> > to consider the destination format as independent.
> 
> +1 for keeping it separate.

Just like for the stateless decoding API, it would make sense to expect
userspace to set those before enumerating CAPTURE formats in order to
determine what the hardware can output.

> > * Custom Huffman tables (DC and AC), both for luma and chroma
> > 
> > It seems that there is a default table when no Huffman table is provided
> > in the bitstream (I'm not too sure how standard that is, just started
> > learning about JPEG). We probably need a specific control for that.
> 
> What happens if there is one in the bitstream? Would the hardware pick
> it automatically?

In our case, this part of the bitstream wouldn't be sent to the
hardware anyway.

> I think it might make sense to just have a general control for Huffman
> table, which would be always provided by the user space, regardless of
> whether it's parsed from the stream or default, so that drivers don't
> have to care and could just always use it.

For MPEG-2 support (and probably also H.265), we have considered the
quantization tables optional and kept a default value in the driver.
That's because said tables are not supported in all profiles, so they
are de-facto optional. I think it's fair to consider that userspace
does not need to implement more than what is needed for decoding. This
makes our interface closer to the data obtained from the bitstream.

However, having one copy of the default table per driver is far from
optimal. I would suggest moving it to common v4l2 functions instead,
but keeping it in kernel space.

What do you think?

Cheers,

Paul

-- 
Developer of free digital technology and hardware support.

Website: https://www.paulk.fr/
Coding blog: https://code.paulk.fr/
Git repositories: https://git.paulk.fr/ https://git.code.paulk.fr/

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 200 bytes --]

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip