[0/5] v4l2 JPEG helpers and CODA960 JPEG decoder
mbox series

Message ID 20191113150538.9807-1-p.zabel@pengutronix.de
Headers show
Series
  • v4l2 JPEG helpers and CODA960 JPEG decoder
Related show

Message

Philipp Zabel Nov. 13, 2019, 3:05 p.m. UTC
Hi,

as far as I can tell we currently have three JPEG header parsers in the
media tree (in the rcar_jpu, s5p-jpeg, and mtk-jpeg drivers). I would
like to add support for the CODA960 JPEG decoder to the coda-vpu driver
without adding yet another.

To this end, this patch series adds some common JPEG code to v4l2-core.
For now this just contains header parsing helpers (I have tried to keep
the terminology close to JPEG ITU-T.81) that should be usable for all of
the current drivers. In the future we might want to move JPEG header
generation for encoders and common quantization tables in there as well.

I have tested this on hardware only with coda-vpu, the other drivers are
just compile-tested.

Feedback very welcome, especially whether this actually works for the
other drivers, and if this could be structured any better. I'm a bit
unhappy with the (current) need for separate frame/scan header and
quantization/hfufman table parsing functions, but those are required
by s5p-jpeg, which splits localization and parsing of the marker
segments. Also, could this be used for i.MX8 JPEGDEC as is?

regards
Philipp

Philipp Zabel (5):
  media: add v4l2 JPEG helpers
  media: coda: jpeg: add CODA960 JPEG decoder support
  media: rcar_jpu: use V4L2 JPEG helpers
  media: s5p-jpeg: use v4l2 JPEG helpers
  media: mtk-jpeg: use V4L2 JPEG helpers

 drivers/media/platform/Kconfig                |   4 +
 drivers/media/platform/coda/coda-common.c     | 124 +++-
 drivers/media/platform/coda/coda-jpeg.c       | 551 ++++++++++++++++
 drivers/media/platform/coda/coda.h            |  11 +-
 .../media/platform/mtk-jpeg/mtk_jpeg_parse.c  | 138 +---
 drivers/media/platform/rcar_jpu.c             |  94 +--
 drivers/media/platform/s5p-jpeg/jpeg-core.c   | 388 +++--------
 drivers/media/platform/s5p-jpeg/jpeg-core.h   |  14 +-
 drivers/media/v4l2-core/Kconfig               |   4 +
 drivers/media/v4l2-core/Makefile              |   2 +
 drivers/media/v4l2-core/v4l2-jpeg.c           | 614 ++++++++++++++++++
 include/media/v4l2-jpeg.h                     | 135 ++++
 12 files changed, 1580 insertions(+), 499 deletions(-)
 create mode 100644 drivers/media/v4l2-core/v4l2-jpeg.c
 create mode 100644 include/media/v4l2-jpeg.h

Comments

Ezequiel Garcia Nov. 13, 2019, 7:42 p.m. UTC | #1
Hi Philipp,

Thanks a lot for working on this. I'm so glad about
both efforts: the CODA JPEG support and the JPEG
helpers.

On Wed, 2019-11-13 at 16:05 +0100, Philipp Zabel wrote:
> Hi,
> 
> as far as I can tell we currently have three JPEG header parsers in the
> media tree (in the rcar_jpu, s5p-jpeg, and mtk-jpeg drivers). I would
> like to add support for the CODA960 JPEG decoder to the coda-vpu driver
> without adding yet another.
> 
> To this end, this patch series adds some common JPEG code to v4l2-core.
> For now this just contains header parsing helpers (I have tried to keep
> the terminology close to JPEG ITU-T.81) that should be usable for all of
> the current drivers. In the future we might want to move JPEG header
> generation for encoders and common quantization tables in there as well.
> 

Indeed, moving encoders to use these helpers sounds like the right thing
to do. IIRC, quantization tables are implementation defined, and not imposed
by anything. I believe gspca drivers use some tables that don't follow
any recomendation.

I guess it's fine to leave some drivers unconverted, without using any helpers,
and move others to use a helper-derived quantization table.  

> I have tested this on hardware only with coda-vpu, the other drivers are
> just compile-tested.
> 
> Feedback very welcome, especially whether this actually works for the
> other drivers, and if this could be structured any better. I'm a bit
> unhappy with the (current) need for separate frame/scan header and
> quantization/hfufman table parsing functions, but those are required
> by s5p-jpeg, which splits localization and parsing of the marker
> segments. Also, could this be used for i.MX8 JPEGDEC as is?
> 
[..]

Regards,
Ezequiel
Jacek Anaszewski Nov. 13, 2019, 8:36 p.m. UTC | #2
Hi Philipp, Ezequiel,

On 11/13/19 8:42 PM, Ezequiel Garcia wrote:
> Hi Philipp,
> 
> Thanks a lot for working on this. I'm so glad about
> both efforts: the CODA JPEG support and the JPEG
> helpers.
> 
> On Wed, 2019-11-13 at 16:05 +0100, Philipp Zabel wrote:
>> Hi,
>>
>> as far as I can tell we currently have three JPEG header parsers in the
>> media tree (in the rcar_jpu, s5p-jpeg, and mtk-jpeg drivers). I would
>> like to add support for the CODA960 JPEG decoder to the coda-vpu driver
>> without adding yet another.
>>
>> To this end, this patch series adds some common JPEG code to v4l2-core.
>> For now this just contains header parsing helpers (I have tried to keep
>> the terminology close to JPEG ITU-T.81) that should be usable for all of
>> the current drivers. In the future we might want to move JPEG header
>> generation for encoders and common quantization tables in there as well.
>>
> 
> Indeed, moving encoders to use these helpers sounds like the right thing
> to do. IIRC, quantization tables are implementation defined, and not imposed
> by anything. I believe gspca drivers use some tables that don't follow
> any recomendation.
> 
> I guess it's fine to leave some drivers unconverted, without using any helpers,
> and move others to use a helper-derived quantization table.  

I fully agree here. I don't have longer access to Exynos4x12 and 3250
based boards to test the patches and the volume of changes allows
to assume that not everything will go smoothly. That can lead to
unpleasant hangups during decoding, caused by not arrived IRQ when
e.g. unsupported JPEG->raw format subsampling transition is requested.

>> I have tested this on hardware only with coda-vpu, the other drivers are
>> just compile-tested.
>>
>> Feedback very welcome, especially whether this actually works for the
>> other drivers, and if this could be structured any better. I'm a bit
>> unhappy with the (current) need for separate frame/scan header and
>> quantization/hfufman table parsing functions, but those are required
>> by s5p-jpeg, which splits localization and parsing of the marker
>> segments. Also, could this be used for i.MX8 JPEGDEC as is?
>>
> [..]
> 
> Regards,
> Ezequiel
> 
>
Nicolas Dufresne Nov. 13, 2019, 9:25 p.m. UTC | #3
Le mercredi 13 novembre 2019 à 21:36 +0100, Jacek Anaszewski a écrit :
> Hi Philipp, Ezequiel,
> 
> On 11/13/19 8:42 PM, Ezequiel Garcia wrote:
> > Hi Philipp,
> > 
> > Thanks a lot for working on this. I'm so glad about
> > both efforts: the CODA JPEG support and the JPEG
> > helpers.
> > 
> > On Wed, 2019-11-13 at 16:05 +0100, Philipp Zabel wrote:
> > > Hi,
> > > 
> > > as far as I can tell we currently have three JPEG header parsers in the
> > > media tree (in the rcar_jpu, s5p-jpeg, and mtk-jpeg drivers). I would
> > > like to add support for the CODA960 JPEG decoder to the coda-vpu driver
> > > without adding yet another.
> > > 
> > > To this end, this patch series adds some common JPEG code to v4l2-core.
> > > For now this just contains header parsing helpers (I have tried to keep
> > > the terminology close to JPEG ITU-T.81) that should be usable for all of
> > > the current drivers. In the future we might want to move JPEG header
> > > generation for encoders and common quantization tables in there as well.
> > > 
> > 
> > Indeed, moving encoders to use these helpers sounds like the right thing
> > to do. IIRC, quantization tables are implementation defined, and not imposed
> > by anything. I believe gspca drivers use some tables that don't follow
> > any recomendation.
> > 
> > I guess it's fine to leave some drivers unconverted, without using any helpers,
> > and move others to use a helper-derived quantization table.  
> 
> I fully agree here. I don't have longer access to Exynos4x12 and 3250
> based boards to test the patches and the volume of changes allows
> to assume that not everything will go smoothly. That can lead to
> unpleasant hangups during decoding, caused by not arrived IRQ when
> e.g. unsupported JPEG->raw format subsampling transition is requested.

My experience with the exynos jpeg decoder was that they are not very
well tested. So better leave them like this until someone have the time
to stabilise them and renew the code a bit.

regards,
Nicolas

> 
> > > I have tested this on hardware only with coda-vpu, the other drivers are
> > > just compile-tested.
> > > 
> > > Feedback very welcome, especially whether this actually works for the
> > > other drivers, and if this could be structured any better. I'm a bit
> > > unhappy with the (current) need for separate frame/scan header and
> > > quantization/hfufman table parsing functions, but those are required
> > > by s5p-jpeg, which splits localization and parsing of the marker
> > > segments. Also, could this be used for i.MX8 JPEGDEC as is?
> > > 
> > [..]
> > 
> > Regards,
> > Ezequiel
> > 
> >
Philipp Zabel Nov. 14, 2019, 10 a.m. UTC | #4
Hi Ezequiel,

On Wed, 2019-11-13 at 16:42 -0300, Ezequiel Garcia wrote:
> Hi Philipp,
> 
> Thanks a lot for working on this. I'm so glad about
> both efforts: the CODA JPEG support and the JPEG
> helpers.
> 
> On Wed, 2019-11-13 at 16:05 +0100, Philipp Zabel wrote:
> > Hi,
> > 
> > as far as I can tell we currently have three JPEG header parsers in the
> > media tree (in the rcar_jpu, s5p-jpeg, and mtk-jpeg drivers). I would
> > like to add support for the CODA960 JPEG decoder to the coda-vpu driver
> > without adding yet another.
> > 
> > To this end, this patch series adds some common JPEG code to v4l2-core.
> > For now this just contains header parsing helpers (I have tried to keep
> > the terminology close to JPEG ITU-T.81) that should be usable for all of
> > the current drivers. In the future we might want to move JPEG header
> > generation for encoders and common quantization tables in there as well.
> > 
> 
> Indeed, moving encoders to use these helpers sounds like the right thing
> to do. IIRC, quantization tables are implementation defined, and not imposed
> by anything. I believe gspca drivers use some tables that don't follow
> any recomendation.

Right. I was just thinking of the default tables from Annex K.3. I'll
have to recheck what's up with the CODA q tables, but the Huffman tables
are identical between hantro_jpeg and coda-jpeg. I suppose we could make
the tables userspace controlled for those drivers that support arbitrary
ones.

> I guess it's fine to leave some drivers unconverted, without using any helpers,
> and move others to use a helper-derived quantization table.  

Agreed.

regards
Philipp
Mirela Rabulea Nov. 25, 2019, 11:36 a.m. UTC | #5
Hi Philipp,
nice initiative :)

BTW, I could not apply the patch series on linux-next repo.
I applied manually (patch -p1) the #1 patch. The subsequent patches
fail to apply even manually. I'm interested in patch #1 and #4.

Comments below...
 
On Mi, 2019-11-13 at 16:05 +0100, Philipp Zabel wrote:

> the current drivers. In the future we might want to move JPEG header
> generation for encoders and common quantization tables in there as
> well.
For i.MX8, it is necessary sometimes to patch the input jpeg stream,
even for the decoder, due to some limitations in the hardware (example:
only component IDs between 0-3 or 1-4 are supported)
> 
> segments. Also, could this be used for i.MX8 JPEGDEC as is?
> 
> regards
> Philipp
> 

It is useful, I tried it, but it will not work exactly "as is". I'm
sending my initial thoughts as a reply on patch #1

Regards,
Mirela
Adrian Ratiu Dec. 4, 2019, 10:30 a.m. UTC | #6
Hi,

On Wed, 13 Nov 2019, Philipp Zabel <p.zabel@pengutronix.de> wrote:
> Hi, 
> 
> as far as I can tell we currently have three JPEG header parsers 
> in the media tree (in the rcar_jpu, s5p-jpeg, and mtk-jpeg 
> drivers). I would like to add support for the CODA960 JPEG 
> decoder to the coda-vpu driver without adding yet another. 
> 
> To this end, this patch series adds some common JPEG code to 
> v4l2-core.  For now this just contains header parsing helpers (I 
> have tried to keep the terminology close to JPEG ITU-T.81) that 
> should be usable for all of the current drivers. In the future 
> we might want to move JPEG header generation for encoders and 
> common quantization tables in there as well. 
> 
> I have tested this on hardware only with coda-vpu, the other 
> drivers are just compile-tested.

Tested-by: Adrian Ratiu <adrian.ratiu@collabora.com>

I'm testing this series on some i.MX 6 boards I have laying around 
and it works well: the new dev nodes appear once the patched coda 
driver is loaded and gstreamer1.0-plugins-good-jpeg uses them for 
dec/enc.

Thanks,
Adrian

>
> Feedback very welcome, especially whether this actually works for the
> other drivers, and if this could be structured any better. I'm a bit
> unhappy with the (current) need for separate frame/scan header and
> quantization/hfufman table parsing functions, but those are required
> by s5p-jpeg, which splits localization and parsing of the marker
> segments. Also, could this be used for i.MX8 JPEGDEC as is?
>
> regards
> Philipp
>
> Philipp Zabel (5):
>   media: add v4l2 JPEG helpers
>   media: coda: jpeg: add CODA960 JPEG decoder support
>   media: rcar_jpu: use V4L2 JPEG helpers
>   media: s5p-jpeg: use v4l2 JPEG helpers
>   media: mtk-jpeg: use V4L2 JPEG helpers
>
>  drivers/media/platform/Kconfig                |   4 +
>  drivers/media/platform/coda/coda-common.c     | 124 +++-
>  drivers/media/platform/coda/coda-jpeg.c       | 551 ++++++++++++++++
>  drivers/media/platform/coda/coda.h            |  11 +-
>  .../media/platform/mtk-jpeg/mtk_jpeg_parse.c  | 138 +---
>  drivers/media/platform/rcar_jpu.c             |  94 +--
>  drivers/media/platform/s5p-jpeg/jpeg-core.c   | 388 +++--------
>  drivers/media/platform/s5p-jpeg/jpeg-core.h   |  14 +-
>  drivers/media/v4l2-core/Kconfig               |   4 +
>  drivers/media/v4l2-core/Makefile              |   2 +
>  drivers/media/v4l2-core/v4l2-jpeg.c           | 614 ++++++++++++++++++
>  include/media/v4l2-jpeg.h                     | 135 ++++
>  12 files changed, 1580 insertions(+), 499 deletions(-)
>  create mode 100644 drivers/media/v4l2-core/v4l2-jpeg.c
>  create mode 100644 include/media/v4l2-jpeg.h
>
> -- 
> 2.20.1
Hans Verkuil Dec. 13, 2019, 9:18 a.m. UTC | #7
On 11/13/19 4:05 PM, Philipp Zabel wrote:
> Hi,
> 
> as far as I can tell we currently have three JPEG header parsers in the
> media tree (in the rcar_jpu, s5p-jpeg, and mtk-jpeg drivers). I would
> like to add support for the CODA960 JPEG decoder to the coda-vpu driver
> without adding yet another.
> 
> To this end, this patch series adds some common JPEG code to v4l2-core.
> For now this just contains header parsing helpers (I have tried to keep
> the terminology close to JPEG ITU-T.81) that should be usable for all of
> the current drivers. In the future we might want to move JPEG header
> generation for encoders and common quantization tables in there as well.
> 
> I have tested this on hardware only with coda-vpu, the other drivers are
> just compile-tested.
> 
> Feedback very welcome, especially whether this actually works for the
> other drivers, and if this could be structured any better. I'm a bit
> unhappy with the (current) need for separate frame/scan header and
> quantization/hfufman table parsing functions, but those are required
> by s5p-jpeg, which splits localization and parsing of the marker
> segments. Also, could this be used for i.MX8 JPEGDEC as is?

This series no longer applies, and I gather that you want to make a v2
anyway based on Mirela's comments.

Marked as 'Changes Requested' in patchwork.

Regards,

	Hans

> 
> regards
> Philipp
> 
> Philipp Zabel (5):
>   media: add v4l2 JPEG helpers
>   media: coda: jpeg: add CODA960 JPEG decoder support
>   media: rcar_jpu: use V4L2 JPEG helpers
>   media: s5p-jpeg: use v4l2 JPEG helpers
>   media: mtk-jpeg: use V4L2 JPEG helpers
> 
>  drivers/media/platform/Kconfig                |   4 +
>  drivers/media/platform/coda/coda-common.c     | 124 +++-
>  drivers/media/platform/coda/coda-jpeg.c       | 551 ++++++++++++++++
>  drivers/media/platform/coda/coda.h            |  11 +-
>  .../media/platform/mtk-jpeg/mtk_jpeg_parse.c  | 138 +---
>  drivers/media/platform/rcar_jpu.c             |  94 +--
>  drivers/media/platform/s5p-jpeg/jpeg-core.c   | 388 +++--------
>  drivers/media/platform/s5p-jpeg/jpeg-core.h   |  14 +-
>  drivers/media/v4l2-core/Kconfig               |   4 +
>  drivers/media/v4l2-core/Makefile              |   2 +
>  drivers/media/v4l2-core/v4l2-jpeg.c           | 614 ++++++++++++++++++
>  include/media/v4l2-jpeg.h                     | 135 ++++
>  12 files changed, 1580 insertions(+), 499 deletions(-)
>  create mode 100644 drivers/media/v4l2-core/v4l2-jpeg.c
>  create mode 100644 include/media/v4l2-jpeg.h
>