| Message ID | 20191113150538.9807-1-p.zabel@pengutronix.de (mailing list archive) |
|---|---|
| Headers | show |
| Series | v4l2 JPEG helpers and CODA960 JPEG decoder | expand |
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
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 > >
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 > > > >
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
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
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
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 >
Hi Philipp, Further testing revealed the decoder rejects jpegs with optimized huffman tables, but the following decoder patch makes them work. Feel free to include these changes in your next version. Adrian 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. > > 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
Hi Adrian, W dniu 18.03.2020 o 11:41, Adrian Ratiu pisze: > Hi Philipp, > > Further testing revealed the decoder rejects jpegs with optimized huffman > tables, but the following decoder patch makes them work. > > Feel free to include these changes in your next version. > > > Adrian > > > drivers/media/platform/coda/coda-jpeg.c | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-)>> > > diff --git a/drivers/media/platform/coda/coda-jpeg.c b/drivers/media/platform <snip> > } > - if (huffman_tables[i].length != ((i & 2) ? 178 : 28)) { > + if (huffman_tables[i].length < 17) { > v4l2_err(&dev->v4l2_dev, > "invalid Huffman table %d length: %zu\n", i, > huffman_tables[i].length); Shouldn't you also be checking the upper bound on the table length, to ensure that you won't exceed the memcpy() destination's capacity below? <snip> > - memcpy(huff_tab->dc_values[0], huffman_tables[0].start + 16, 12); > + memcpy(huff_tab->dc_values[0], huffman_tables[0].start + 16, huffman_tables[0].length - 16); Andrzej
Hi Andrzej, On Wed, 18 Mar 2020, Andrzej Pietrasiewicz <andrzejtp2010@gmail.com> wrote: > Hi Adrian, > > W dniu 18.03.2020 o 11:41, Adrian Ratiu pisze: > > Hi Philipp, > > > > Further testing revealed the decoder rejects jpegs with > > optimized huffman tables, but the following decoder patch > > makes them work. > > > > Feel free to include these changes in your next version. > > > > Adrian > > > > drivers/media/platform/coda/coda-jpeg.c | 10 +++++----- 1 > > file changed, 5 insertions(+), 5 deletions(-)>> > > > > diff --git a/drivers/media/platform/coda/coda-jpeg.c > > b/drivers/media/platform > <snip> > > > } - if (huffman_tables[i].length != > > ((i & 2) ? 178 : 28)) { + if > > (huffman_tables[i].length < 17) { v4l2_err(&dev->v4l2_dev, > > "invalid Huffman table %d length: %zu\n", i, > > huffman_tables[i].length); > > Shouldn't you also be checking the upper bound on the table > length, to ensure that you won't exceed the memcpy() > destination's capacity below? Good point, it's always good to have an upper bound sanity check test, even though in practice the optimized tables are smaller than the std ones by definition, there are never enough checks against bugs :) Thanks, Adrian > > <snip> > > > - memcpy(huff_tab->dc_values[0], huffman_tables[0].start + 16, 12); > > + memcpy(huff_tab->dc_values[0], huffman_tables[0].start + 16, > huffman_tables[0].length - 16); > > > Andrzej