Re: [EXT] [PATCH 1/5] media: add v4l2 JPEG helpers

[Mirela Rabulea <mirela.rabulea@nxp.com>]

Hi Philipp,

On Lu, 2019-11-25 at 17:36 +0100, Philipp Zabel wrote:
> 
> 
> > 1. It is necessary to support ARGB (4 components)
> Ok. I'll add support for 4-component images.
Thanks.
>  Is there a reasonable
> default color encoding for 4-component images if the APP14 marker
> segment is not present?
Besides APP14, I did not find anything else that works, without APP14,
RGB/ARGB colors are distorted, the only essplanation for this was the
one from Rec T.872.
> 
> > 
> > 2. It is necessary to support extended sequential (parse SOF1)
> Do you require arithmetic coding or 12-bit sample precision as well,
> or
> just extended sequential with Huffman coding and 8-bit sample
> precision?
The imx8 jpeg supports both 8-bit and 12-bit sample precision. I only
tested with 8-bit samples, some little adjustments might be necessary
for 12-bit in the imx8 jpeg driver, having something for it in the
common code would make it easier.


> I see you used the "APP14 marker segment for colour encoding" as
> specified in Recommendation T.872 [1]. I'll add support for this to
> the
> common code.
Thanks.
> 
> > 
> > 4. It is necessary to be able to modify/patch the component ID's
> > inside
> > SOF & SOS segments; this is due to a hardware limitation that the
> > component ID's must be 0..3 or 1..4, however it is possible to
> > decode a
> > jpeg that violates this condition, if the component ID's are
> > patched to
> > accepted values.
> Interesting. I'm not sure if this is something we should do
> unconditionally in the common code. Maybe this warrants a flag.
I forgot to mention, mxc_jpeg_valid_comp_id is where I did this hack,
and that patching of the component IDs happens directly over the source
(OUTPUT) buffer. If there won't be a helper for it, I will still have
to be able to parse the SOF/SOS segments, which I was hoping to drop
after using the common helpers.
> 
> > 
> > I have a concern related to performance, about parsing the jpeg
> > like
> > that, but I did not get to measure anything yet, as I could not
> > fully
> > integrate imx8 jpeg driver with the helpers, I
> > used v4l2_jpeg_parse_header, but I also had to keep my old
> > structures.
> > Please take a look in my imx8 patch, at mxc-jpeg.h, struct
> > mxc_jpeg_sof/struct mxc_jpeg_sos, these are __packed structures,
> > they
> > work quite well via a simple cast and allow modifications too, the
> > downside is that fields bigger than u8 might require swapping.
> We can't use bitfields for external data in portable code, and I'm
> not a
> big fan of using __be16 in the API, but we could certainly use this
> internally and see if that speeds up parsing. There are quite a few
> superfluous bounds checks right now that can be optimized away.
> I'd still like to copy the parsed headers into a structure provided
> by
> the caller.
Ok, I'll try to do some measurements after the next version, with
versus without the helpers.
> 
> > 
> > Please also see below my comments.
> I'll take these into account for the next version. Thank you for the
> feedback!
Thank you!
> 
> regards
> Philipp
>