Re: width and height of JPEG compressed images

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Sylwester,

On Sunday 01 September 2013 00:25:56 Sylwester Nawrocki wrote:
> Hi Thomas,
> 
> I sincerely apologise for not having replied earlier. Looks like I'm
> being pulled in too many directions. :/
> 
> On 08/06/2013 06:26 PM, Thomas Vajzovic wrote:
> > On 24 July 2013 10:30 Sylwester Nawrocki wrote:
> >> On 07/22/2013 10:40 AM, Thomas Vajzovic wrote:
> >>> On 21 July 2013 21:38 Sylwester Nawrocki wrote:
> >>>> On 07/19/2013 10:28 PM, Sakari Ailus wrote:
> >>>>> On Sat, Jul 06, 2013 at 09:58:23PM +0200, Sylwester Nawrocki wrote:
> >>>>>> On 07/05/2013 10:22 AM, Thomas Vajzovic wrote:
> >>>>>>> The hardware reads AxB sensor pixels from its array, resamples
> >>>>>>> them to CxD image pixels, and then compresses them to ExF bytes.
> >>> 
> >>> If the sensor driver is only told the user's requested sizeimage, it
> >>> can be made to factorize (ExF) into (E,F) itself, but then both the
> >>> parallel interface and the 2D DMA peripheral need to be told the
> >>> particular factorization that it has chosen.
> >>> 
> >>> If the user requests sizeimage which cannot be satisfied (eg: a prime
> >>> number) then it will need to return (E,F) to the bridge driver which
> >>> does not multiply exactly to sizeimage.  Because of this the bridge
> >>> driver must set the corrected value of sizeimage which it returns to
> >>> userspace to the product ExF.
> >> 
> >> Ok, let's consider following data structure describing the frame:
> >> 
> >> struct v4l2_frame_desc_entry {
> >>    u32 flags;
> >>    u32 pixelcode;
> >>    u32 samples_per_line;
> >>    u32 num_lines;
> >>    u32 size;
> >> };
> >> 
> >> I think we could treat the frame descriptor to be at lower lever in
> >> the protocol stack than struct v4l2_mbus_framefmt.
> >> 
> >> Then the bridge would set size and pixelcode and the subdev would
> >> return (E, F) in (samples_per_frame, num_lines) and adjust size if
> >> required. Number of bits per sample can be determined by pixelcode.
> >> 
> >> It needs to be considered that for some sensor drivers it might not
> >> be immediately clear what samples_per_line, num_lines values are.
> >> In such case those fields could be left zeroed and bridge driver
> >> could signal such condition as a more or less critical error. In
> >> end of the day specific sensor driver would need to be updated to
> >> interwork with a bridge that requires samples_per_line, num_lines.
> > 
> > I think we ought to try to consider the four cases:
> > 
> > 1D sensor and 1D bridge: already works
> > 
> > 2D sensor and 2D bridge: my use case
> > 
> > 1D sensor and 2D bridge, 2D sensor and 1D bridge:
> > Perhaps both of these cases could be made to work by setting:
> > num_lines = 1; samples_per_line = ((size * 8) / bpp);
> > 
> > (Obviously this would also require the appropriate pull-up/down
> > on the second sync input on a 2D bridge).
> 
> So to determine when this has to be done, e.g. we could see that either
> num_lines or samples_per_line == 1 ?
> 
> > Since the frame descriptor interface is still new and used in so
> > few drivers, is it reasonable to expect them all to be fixed to
> > do this?
> 
> Certainly, I'll be happy to rework those drivers to whatever the
> re-designed API, as long as it supports the current functionality.
> 
> >> Not sure if we need to add image width and height in pixels to the
> >> above structure. It wouldn't make much sensor when single frame
> >> carries multiple images, e.g. interleaved YUV and compressed image
> >> data at different resolutions.
> > 
> > If image size were here then we are duplicating get_fmt/set_fmt.
> > But then, by having pixelcode here we are already duplicating part
> > of get_fmt/set_fmt.  If the bridge changes pixelcode and calls
> > set_frame_desc then is this equivalent to calling set_fmt?
> > I would like to see as much data normalization as possible and
> > eliminate the redundancy.
> 
> Perhaps we could replace pixelcode in the above struct by something
> else that would have conveyed required data. But I'm not sure what it
> would have been. Perhaps just bits_per_sample for starters ?
> 
> The frame descriptors were also supposed to cover interleaved image data.
> Then we need something like pixelcode (MIPI CSI-2 Data Type) in each
> frame_desc entry.
> 
> Not sure if it would have been sensible to put some of the above
> information into struct v4l2_mbus_frame_desc rather than to struct
> v4l2_mbus_frame_desc_entry.
> 
> >>> Whatever mechanism is chosen needs to have corresponding get/set/try
> >>> methods to be used when the user calls
> >>> VIDIOC_G_FMT/VIDIOC_S_FMT/VIDIOC_TRY_FMT.
> >> 
> >> Agreed, it seems we need some sort of negotiation of those low
> >> level parameters.
> > 
> > Should there be set/get/try function pointers, or should the struct
> > include an enum member like v4l2_subdev_format.which to determine
> > which operation is to be perfomed?
> > 
> > Personally I think that it is a bit ugly having two different
> > function pointers for set_fmt/get_fmt but then a structure member
> > to determine between set/try.  IMHO it should be three function
> > pointers or one function with a three valued enum in the struct.
> 
> I'm fine either way, with a slight preference for three separate callbacks.

Don't forget that the reason the pad-level API was designed this way is that 
trying a format on a pad requires configuring formats on all other pads. Pads 
are not isolated, they depend on each other, so we can't have a standalone try 
operation.

> WRT to making frame_desc read-only, subdevices for which it doesn't make
> sense to set anything could always adjust passed data to their fixed or
> depending on other calls, like the pad level set_fmt op, values. And we
> seem to have use cases already for at least try_frame_desc.

-- 
Regards,

Laurent Pinchart