width and height of JPEG compressed images

width and height of JPEG compressed images

[Thomas Vajzovic]

Hello,

I am writing a driver for the sensor MT9D131.  This device supports digital zoom and JPEG compression.

Although I am writing it for my company's internal purposes, it will be made open-source, so I would like to keep the API as portable as possible.

The hardware reads AxB sensor pixels from its array, resamples them to CxD image pixels, and then compresses them to ExF bytes.

The subdevice driver sets size AxB to the value it receives from v4l2_subdev_video_ops.s_crop().

To enable compression then v4l2_subdev_video_ops.s_mbus_fmt() is called with fmt->code=V4L2_MBUS_FMT_JPEG_1X8.

fmt->width and fmt->height then ought to specify the size of the compressed image ExF, that is, the size specified is the size in the format specified (the number of JPEG_1X8), not the size it would be in a raw format.

This allows the bridge driver to be compression agnostic.  It gets told how many bytes to allocate per buffer and it reads that many bytes.  It doesn't have to understand that the number of bytes isn't directly related to the number of pixels.

So how does the user tell the driver what size image to capture before compression, CxD?

(or alternatively, if you disagree and think CxD should be specified by s_fmt(), then how does the user specify ExF?)

Regards,
Tom

--
Mr T. Vajzovic
Software Engineer
Infrared Integrated Systems Ltd
Visit us at www.irisys.co.uk
Disclaimer: This e-mail message is confidential and for use by the addressee only. If the message is received by anyone other than the addressee, please return the message to the sender by replying to it and then delete the original message and the sent message from your computer. Infrared Integrated Systems Limited Park Circle Tithe Barn Way Swan Valley Northampton NN4 9BG Registration Number: 3186364.

Re: width and height of JPEG compressed images

[Sylwester Nawrocki]

Hi Thomas,

Cc: Sakari and Laurent

On 07/05/2013 10:22 AM, Thomas Vajzovic wrote:
> Hello,
>
> I am writing a driver for the sensor MT9D131.  This device supports
> digital zoom and JPEG compression.
>
> Although I am writing it for my company's internal purposes, it will
>be made open-source, so I would like to keep the API as portable as
> possible.
>
> The hardware reads AxB sensor pixels from its array, resamples them
>to CxD image pixels, and then compresses them to ExF bytes.
>
> The subdevice driver sets size AxB to the value it receives from
>v4l2_subdev_video_ops.s_crop().
>
> To enable compression then v4l2_subdev_video_ops.s_mbus_fmt() is
>called with fmt->code=V4L2_MBUS_FMT_JPEG_1X8.
>
> fmt->width and fmt->height then ought to specify the size of the
>compressed image ExF, that is, the size specified is the size in the
>format specified (the number of JPEG_1X8), not the size it would be
>in a raw format.

In VIDIOC_S_FMT 'sizeimage' specifies size of the buffer for the
compressed frame at the bridge driver side. And width/height should
specify size of the re-sampled (binning, skipping ?) frame - CxD,
if I understand what  you are saying correctly.

I don't quite what transformation is done at CxD -> ExF. Why you are
using ExF (two numbers) to specify number of bytes ? And how can you
know exactly beforehand what is the frame size after compression ?
Does the sensor transmit fixed number of bytes per frame, by adding
some padding bytes if required to the compressed frame data ?

Is it something like:

sensor matrix (AxB pixels) -> binning/skipping (CxD pixels) ->
-> JPEG compresion (width = C, height = D, sizeimage ExF bytes)

?
> This allows the bridge driver to be compression agnostic.  It gets
>told how many bytes to allocate per buffer and it reads that many
>bytes.  It doesn't have to understand that the number of bytes isn't
>directly related to the number of pixels.
>
> So how does the user tell the driver what size image to capture
>before compression, CxD?

I think you should use VIDIOC_S_FMT(width = C, height = D, sizeimage = ExF)
for that. And s_frame_desc sudev op could be used to pass sizeimage to the
sensor subdev driver.

> (or alternatively, if you disagree and think CxD should be specified
>by s_fmt(), then how does the user specify ExF?)

Regards,
Sylwester

RE: width and height of JPEG compressed images

[Thomas Vajzovic]

Hi Sylwester,

On 06 July 2013 20:58 Sylwester Nawrocki wrote:
> On 07/05/2013 10:22 AM, Thomas Vajzovic wrote:
>>
>> I am writing a driver for the sensor MT9D131.  This device supports
>> digital zoom and JPEG compression.
>>
>> The hardware reads AxB sensor pixels from its array, resamples them
>> to CxD image pixels, and then compresses them to ExF bytes.
>>
>> fmt->width and fmt->height then ought to specify the size of the
>> compressed image ExF, that is, the size specified is the size in the
>> format specified (the number of JPEG_1X8), not the size it would be
>> in a raw format.
>
> In VIDIOC_S_FMT 'sizeimage' specifies size of the buffer for the
> compressed frame at the bridge driver side. And width/height should
> specify size of the re-sampled (binning, skipping ?) frame - CxD,
> if I understand what  you are saying correctly.
>
> I don't quite what transformation is done at CxD -> ExF. Why you are
> using ExF (two numbers) to specify number of bytes ? And how can you
> know exactly beforehand what is the frame size after compression ?
> Does the sensor transmit fixed number of bytes per frame, by adding
> some padding bytes if required to the compressed frame data ?
>
> Is it something like:
>
> sensor matrix (AxB pixels) -> binning/skipping (CxD pixels) ->
> -> JPEG compresion (width = C, height = D, sizeimage ExF bytes)
>
> I think you should use VIDIOC_S_FMT(width = C, height = D,
> sizeimage = ExF) for that. And s_frame_desc sudev op could be used to
> pass sizeimage to the sensor subdev driver.

Yes you are correct that the sensor zero pads the compressed data to a
fixed size.  That size must be specified in two separate registers,
called spoof width and spoof height.  Above CxD is the image size after
binning/skipping and resizing, ExF is the spoof size.

The reason for two numbers for the number of bytes is that as the
sensor outputs the JPEG bytes the VSYNC and HSYNC lines behave as
though they were still outputting a 2D image with 8bpp.  This means
that no changes are required in the bridge hardware.  I am trying to
make it so very few changes are required in the bridge driver too.
As far as the bridge driver is concerned the only size is ExF, it is
unconcerned with CxD.

I somehow overlooked the member sizeimage.  Having re-read the
documentation I think that in the user<->bridge device the interface
is clear:

v4l2_pix_format.width        = C;
v4l2_pix_format.height       = D;
v4l2_pix_format.bytesperline = E;
v4l2_pix_format.sizeimage    = (E * F);

bytesperline < width
(sizeimage % bytesperline) == 0
(sizeimage / bytesperline) < height

But the question now is how does the bridge device communicate this to
the I2C subdevice?  v4l2_mbus_framefmt doesn't have bytesperline or
sizeimage, and v4l2_mbus_frame_desc_entry has only length (which I
presume is sizeimage) but not both dimensions.

Thanks,
Tom
Disclaimer: This e-mail message is confidential and for use by the addressee only. If the message is received by anyone other than the addressee, please return the message to the sender by replying to it and then delete the original message and the sent message from your computer. Infrared Integrated Systems Limited Park Circle Tithe Barn Way Swan Valley Northampton NN4 9BG Registration Number: 3186364.

Re: width and height of JPEG compressed images

[Sylwester Nawrocki]

Hi Tom,

On 07/07/2013 10:18 AM, Thomas Vajzovic wrote:
> On 06 July 2013 20:58 Sylwester Nawrocki wrote:
>> On 07/05/2013 10:22 AM, Thomas Vajzovic wrote:
>>>
>>> I am writing a driver for the sensor MT9D131.  This device supports
>>> digital zoom and JPEG compression.
>>>
>>> The hardware reads AxB sensor pixels from its array, resamples them
>>> to CxD image pixels, and then compresses them to ExF bytes.
>>>
>>> fmt->width and fmt->height then ought to specify the size of the
>>> compressed image ExF, that is, the size specified is the size in the
>>> format specified (the number of JPEG_1X8), not the size it would be
>>> in a raw format.
>>
>> In VIDIOC_S_FMT 'sizeimage' specifies size of the buffer for the
>> compressed frame at the bridge driver side. And width/height should
>> specify size of the re-sampled (binning, skipping ?) frame - CxD,
>> if I understand what  you are saying correctly.
>>
>> I don't quite what transformation is done at CxD ->  ExF. Why you are
>> using ExF (two numbers) to specify number of bytes ? And how can you
>> know exactly beforehand what is the frame size after compression ?
>> Does the sensor transmit fixed number of bytes per frame, by adding
>> some padding bytes if required to the compressed frame data ?
>>
>> Is it something like:
>>
>> sensor matrix (AxB pixels) ->  binning/skipping (CxD pixels) ->
>> ->  JPEG compresion (width = C, height = D, sizeimage ExF bytes)
>>
>> I think you should use VIDIOC_S_FMT(width = C, height = D,
>> sizeimage = ExF) for that. And s_frame_desc sudev op could be used to
>> pass sizeimage to the sensor subdev driver.
>
> Yes you are correct that the sensor zero pads the compressed data to a
> fixed size.  That size must be specified in two separate registers,
> called spoof width and spoof height.  Above CxD is the image size after
> binning/skipping and resizing, ExF is the spoof size.
>
> The reason for two numbers for the number of bytes is that as the
> sensor outputs the JPEG bytes the VSYNC and HSYNC lines behave as
> though they were still outputting a 2D image with 8bpp.  This means
> that no changes are required in the bridge hardware.  I am trying to
> make it so very few changes are required in the bridge driver too.
> As far as the bridge driver is concerned the only size is ExF, it is
> unconcerned with CxD.

OK, that sounds good.

> I somehow overlooked the member sizeimage.  Having re-read the
> documentation I think that in the user<->bridge device the interface
> is clear:
>
> v4l2_pix_format.width        = C;
> v4l2_pix_format.height       = D;
> v4l2_pix_format.bytesperline = E;
> v4l2_pix_format.sizeimage    = (E * F);
>
> bytesperline<  width
> (sizeimage % bytesperline) == 0
> (sizeimage / bytesperline)<  height

bytesperline has not much meaning for compressed formats at the video
device (DMA) driver side. For compressed streams like JPEG size of the
memory buffer to allocate is normally determined by sizeimage.

'bytesperline' could be less than 'width', that means a "virtual"
bits-per-pixel factor is less than 8. But this factor could (should?) be
configurable e.g. indirectly through V4L2_CID_JPEG_QUALITY control, and
the bridge can query it from the sensor through g_frame_desc subdev op.
The bridge has normally no clue what the compression ratio at the sensor
side is. It could hard code some default "bpp", but then it needs to be
ensured the sensor doesn't transmit more data than the size of allocated
buffer.

> But the question now is how does the bridge device communicate this to
> the I2C subdevice?  v4l2_mbus_framefmt doesn't have bytesperline or
> sizeimage, and v4l2_mbus_frame_desc_entry has only length (which I
> presume is sizeimage) but not both dimensions.

That's a good question. The frame descriptors really need more discussion
and improvement, to also cover use cases as your JPEG sensor.
Currently it is pretty pre-eliminary stuff, used by just a few drivers.
Here is the original RFC from Sakari [1].

Since we can't add bytesperline/sizeimage to struct v4l2_mbus_framefmt
I think struct v4l2_mbus_frame_desc_entry needs to be extended and
interaction between subdev ops like video.{s,g}_mbus_fmt, pad.{set,get}_fmt
needs to be specified.

As a side note, looking at the MT9D131 sensor datasheet I can see it has
preview (Mode A) and capture (Mode B) modes. Are you also planning adding
proper support for switching between those modes ? I'm interested in
supporting this in standard way in V4L2, as lot's of sensors I have been
working with also support such modes.

[1] http://www.mail-archive.com/linux-media@vger.kernel.org/msg43530.html

--
Regards,
Sylwester

RE: width and height of JPEG compressed images

[Thomas Vajzovic]

Hi Sylwester,

On 10 July 2013 20:44 Sylwester Nawrocki wrote:
>On 07/07/2013 10:18 AM, Thomas Vajzovic wrote:
>> On 06 July 2013 20:58 Sylwester Nawrocki wrote:
>>> On 07/05/2013 10:22 AM, Thomas Vajzovic wrote:
>>>>
>>>> I am writing a driver for the sensor MT9D131.  This device supports
>>>> digital zoom and JPEG compression.
>>>>
>>>> The hardware reads AxB sensor pixels from its array, resamples them
>>>> to CxD image pixels, and then compresses them to ExF bytes.
>>
>> Yes you are correct that the sensor zero pads the compressed data to a
>> fixed size.  That size must be specified in two separate registers,
>> called spoof width and spoof height.  Above CxD is the image size
>> after binning/skipping and resizing, ExF is the spoof size.
>>
>> The reason for two numbers for the number of bytes is that as the
>> sensor outputs the JPEG bytes the VSYNC and HSYNC lines behave as
>> though they were still outputting a 2D image with 8bpp.  This means
>> that no changes are required in the bridge hardware.  I am trying to
>> make it so very few changes are required in the bridge driver too.
>> As far as the bridge driver is concerned the only size is ExF, it is
>> unconcerned with CxD.
>>
>> v4l2_pix_format.width        = C;
>> v4l2_pix_format.height       = D;
>> v4l2_pix_format.bytesperline = E;
>> v4l2_pix_format.sizeimage    = (E * F);
>>
>> bytesperline < width
>> (sizeimage % bytesperline) == 0
>> (sizeimage / bytesperline) < height
>
> bytesperline has not much meaning for compressed formats at the video
> device (DMA) driver side.

This is not true for the platform I am using.

The Blackfin has a 2D DMA peripheral, meaning that it does need to
separately know bytesperline and (sizeimage / bytesperline).

These values have a physical hardware meaning in terms of the signals
on the sync lines even though they do not have a logical meaning
because the data is compressed.

> For compressed streams like JPEG size of the memory buffer to
> allocate is normally determined by sizeimage.

It is two numbers in my use case.

> 'bytesperline' could be less than 'width', that means a "virtual"
> bits-per-pixel factor is less than 8. But this factor could (should?)
> be configurable e.g. indirectly through V4L2_CID_JPEG_QUALITY control,

This is absolutely not a "virtual" width, it is a real physical property
of the hardware signal.  The hardware signal always has exactly 8 bits
per sample, but its height and width (ExF) are not related to the image
height and width (CxD).

It is not appropriate to group the hardware data size together with
compression controls for two reasons:

Firstly, the bridge driver would need to intercept the control and then
pass it on to the bridge driver because they both need to know E and F.

Secondly, the pair of numbers (E,F) in my case have exaclty the same
meaning and are used in exactly the same way as the single number
(sizeimage) which is used in the cameras that use the current API.
Logically the two numbers should be passed around and set and modified
in all the same places that sizeimage currently is, but as a tuple.
The two cannot be separated with one set using one API and the other
a different API.

> and the bridge can query it from the sensor through g_frame_desc subdev
> op.  The bridge has normally no clue what the compression ratio at the
> sensor side is.  It could hard code some default "bpp", but then it
> needs to be ensured the sensor doesn't transmit more data than the size
> of allocated buffer.

It has no idea what the true compression ratio size is, but it does have
to know the padded size.  The sensor will always send exactly that size.

>> But the question now is how does the bridge device communicate this to
>> the I2C subdevice?  v4l2_mbus_framefmt doesn't have bytesperline or
>> sizeimage, and v4l2_mbus_frame_desc_entry has only length (which I
>> presume is sizeimage) but not both dimensions.
>
> That's a good question. The frame descriptors really need more discussion
> and improvement, to also cover use cases as your JPEG sensor.
> Currently it is pretty pre-eliminary stuff, used by just a few drivers.
> Here is the original RFC from Sakari [1].

The version that has made it to kernel.org is much watered down from this
proposal.  It could be suitable for doing what I need if an extra member
were added, or preferably there should be something like:

enum
{
  DMA_1D,
  DMA_2D,
};

union {
  struct {  // Valid if DMA_1D
    u32 size;
  };
  struct {  // Valid if DMA_2D
    u32 width;
    u32 height;
  };
};

> Since we can't add bytesperline/sizeimage to struct v4l2_mbus_framefmt

Isn't this a sensible case for using some of those reserved bytes?

If not, why are they there?

> I think struct v4l2_mbus_frame_desc_entry needs to be extended and
> interaction between subdev ops like video.{s,g}_mbus_fmt,
> pad.{set,get}_fmt needs to be specified.

Failing adding to v4l2_mbus_framefmt, I agree.

I notice also that there is only set_frame_desc and get_frame_desc, and
no try_frame_desc.

In the only bridge driver that currently uses this interface,
fimc-capture, when the user calls VIDIOC_TRY_FMT, then this is
translated to a call to subdev.set_frame_desc.  Isn't this wrong?  I
thought that TRY_* was never meant to modify the actual hardware, but
only fill out the passed structure with what the device would be able
to do, so don't we need also try_frame_desc?

Best regards,
Tom
Disclaimer: This e-mail message is confidential and for use by the addressee only. If the message is received by anyone other than the addressee, please return the message to the sender by replying to it and then delete the original message and the sent message from your computer. Infrared Integrated Systems Limited Park Circle Tithe Barn Way Swan Valley Northampton NN4 9BG Registration Number: 3186364.

Re: width and height of JPEG compressed images

[Sakari Ailus]

Hi Thomas and Sylwester,

Apologies for my late reply.

On Sat, Jul 06, 2013 at 09:58:23PM +0200, Sylwester Nawrocki wrote:
> Hi Thomas,
> 
> Cc: Sakari and Laurent
> 
> On 07/05/2013 10:22 AM, Thomas Vajzovic wrote:
> >Hello,
> >
> >I am writing a driver for the sensor MT9D131.  This device supports
> >digital zoom and JPEG compression.
> >
> >Although I am writing it for my company's internal purposes, it will
> >be made open-source, so I would like to keep the API as portable as
> >possible.
> >
> >The hardware reads AxB sensor pixels from its array, resamples them
> >to CxD image pixels, and then compresses them to ExF bytes.
> >
> >The subdevice driver sets size AxB to the value it receives from
> >v4l2_subdev_video_ops.s_crop().
> >
> >To enable compression then v4l2_subdev_video_ops.s_mbus_fmt() is
> >called with fmt->code=V4L2_MBUS_FMT_JPEG_1X8.
> >
> >fmt->width and fmt->height then ought to specify the size of the
> >compressed image ExF, that is, the size specified is the size in the
> >format specified (the number of JPEG_1X8), not the size it would be
> >in a raw format.
> 
> In VIDIOC_S_FMT 'sizeimage' specifies size of the buffer for the
> compressed frame at the bridge driver side. And width/height should
> specify size of the re-sampled (binning, skipping ?) frame - CxD,
> if I understand what  you are saying correctly.
> 
> I don't quite what transformation is done at CxD -> ExF. Why you are
> using ExF (two numbers) to specify number of bytes ? And how can you
> know exactly beforehand what is the frame size after compression ?
> Does the sensor transmit fixed number of bytes per frame, by adding
> some padding bytes if required to the compressed frame data ?
> 
> Is it something like:
> 
> sensor matrix (AxB pixels) -> binning/skipping (CxD pixels) ->
> -> JPEG compresion (width = C, height = D, sizeimage ExF bytes)
> 
> ?
> >This allows the bridge driver to be compression agnostic.  It gets
> >told how many bytes to allocate per buffer and it reads that many
> >bytes.  It doesn't have to understand that the number of bytes isn't
> >directly related to the number of pixels.
> >
> >So how does the user tell the driver what size image to capture
> >before compression, CxD?
> 
> I think you should use VIDIOC_S_FMT(width = C, height = D, sizeimage = ExF)
> for that. And s_frame_desc sudev op could be used to pass sizeimage to the
> sensor subdev driver.

Agreed. Let me take this into account in the next RFC.

> >(or alternatively, if you disagree and think CxD should be specified
> >by s_fmt(), then how does the user specify ExF?)

Does the user need to specify ExF, for other purposes than limiting the size
of the image? I would leave this up to the sensor driver (with reasonable
alignment). The sensor driver would tell about this to the receiver through
frame descriptors. (But still I don't think frame descriptors should be
settable; what sensors can support is fully sensor specific and the
parameters that typically need to be changed are quite limited in numbers.
So I'd go with e.g. controls, again.)

-- 
Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

Re: width and height of JPEG compressed images

[Sakari Ailus]

Hi Thomas and Sylwester,

On Mon, Jul 15, 2013 at 09:18:36AM +0000, Thomas Vajzovic wrote:
> On 10 July 2013 20:44 Sylwester Nawrocki wrote:
> >On 07/07/2013 10:18 AM, Thomas Vajzovic wrote:
> >> On 06 July 2013 20:58 Sylwester Nawrocki wrote:
> >>> On 07/05/2013 10:22 AM, Thomas Vajzovic wrote:
> >>>>
> >>>> I am writing a driver for the sensor MT9D131.  This device supports
> >>>> digital zoom and JPEG compression.
> >>>>
> >>>> The hardware reads AxB sensor pixels from its array, resamples them
> >>>> to CxD image pixels, and then compresses them to ExF bytes.
> >>
> >> Yes you are correct that the sensor zero pads the compressed data to a
> >> fixed size.  That size must be specified in two separate registers,
> >> called spoof width and spoof height.  Above CxD is the image size
> >> after binning/skipping and resizing, ExF is the spoof size.
> >>
> >> The reason for two numbers for the number of bytes is that as the
> >> sensor outputs the JPEG bytes the VSYNC and HSYNC lines behave as
> >> though they were still outputting a 2D image with 8bpp.  This means
> >> that no changes are required in the bridge hardware.  I am trying to
> >> make it so very few changes are required in the bridge driver too.
> >> As far as the bridge driver is concerned the only size is ExF, it is
> >> unconcerned with CxD.
> >>
> >> v4l2_pix_format.width        = C;
> >> v4l2_pix_format.height       = D;
> >> v4l2_pix_format.bytesperline = E;
> >> v4l2_pix_format.sizeimage    = (E * F);
> >>
> >> bytesperline < width
> >> (sizeimage % bytesperline) == 0
> >> (sizeimage / bytesperline) < height
> >
> > bytesperline has not much meaning for compressed formats at the video
> > device (DMA) driver side.
> 
> This is not true for the platform I am using.
> 
> The Blackfin has a 2D DMA peripheral, meaning that it does need to
> separately know bytesperline and (sizeimage / bytesperline).
> 
> These values have a physical hardware meaning in terms of the signals
> on the sync lines even though they do not have a logical meaning
> because the data is compressed.

Parallel receivers typically do have such configuration but it's new to me
that a DMA controller also does.

In terms of the original RFC, just setting width, height and bpp accordingly
should do the trick. These are the parameters of the physical, not the
image. Choosing the line width wouldn't be possible, but would it be an
issue to use a constant line width?

> > For compressed streams like JPEG size of the memory buffer to
> > allocate is normally determined by sizeimage.
> 
> It is two numbers in my use case.
> 
> > 'bytesperline' could be less than 'width', that means a "virtual"
> > bits-per-pixel factor is less than 8. But this factor could (should?)
> > be configurable e.g. indirectly through V4L2_CID_JPEG_QUALITY control,
> 
> This is absolutely not a "virtual" width, it is a real physical property
> of the hardware signal.  The hardware signal always has exactly 8 bits
> per sample, but its height and width (ExF) are not related to the image
> height and width (CxD).
> 
> It is not appropriate to group the hardware data size together with
> compression controls for two reasons:
> 
> Firstly, the bridge driver would need to intercept the control and then
> pass it on to the bridge driver because they both need to know E and F.
> 
> Secondly, the pair of numbers (E,F) in my case have exaclty the same
> meaning and are used in exactly the same way as the single number
> (sizeimage) which is used in the cameras that use the current API.
> Logically the two numbers should be passed around and set and modified
> in all the same places that sizeimage currently is, but as a tuple.
> The two cannot be separated with one set using one API and the other
> a different API.
> 
> > and the bridge can query it from the sensor through g_frame_desc subdev
> > op.  The bridge has normally no clue what the compression ratio at the
> > sensor side is.  It could hard code some default "bpp", but then it
> > needs to be ensured the sensor doesn't transmit more data than the size
> > of allocated buffer.
> 
> It has no idea what the true compression ratio size is, but it does have
> to know the padded size.  The sensor will always send exactly that size.
> 
> >> But the question now is how does the bridge device communicate this to
> >> the I2C subdevice?  v4l2_mbus_framefmt doesn't have bytesperline or
> >> sizeimage, and v4l2_mbus_frame_desc_entry has only length (which I
> >> presume is sizeimage) but not both dimensions.
> >
> > That's a good question. The frame descriptors really need more discussion
> > and improvement, to also cover use cases as your JPEG sensor.
> > Currently it is pretty pre-eliminary stuff, used by just a few drivers.
> > Here is the original RFC from Sakari [1].
> 
> The version that has made it to kernel.org is much watered down from this
> proposal.  It could be suitable for doing what I need if an extra member
> were added, or preferably there should be something like:
> 
> enum
> {
>   DMA_1D,
>   DMA_2D,
> };
> 
> union {
>   struct {  // Valid if DMA_1D
>     u32 size;
>   };
>   struct {  // Valid if DMA_2D
>     u32 width;
>     u32 height;
>   };
> };
> 
> > Since we can't add bytesperline/sizeimage to struct v4l2_mbus_framefmt
> 
> Isn't this a sensible case for using some of those reserved bytes?

struct v4l2_mbus_framefmt is part of the user space interface and adding
information to it should be done with care. I'm not sure the end user would
be even interested to know (let alone to configure) how the image is
transferred over the bus since this configuration does not affect the
resulting image.

> If not, why are they there?
> 
> > I think struct v4l2_mbus_frame_desc_entry needs to be extended and
> > interaction between subdev ops like video.{s,g}_mbus_fmt,
> > pad.{set,get}_fmt needs to be specified.
> 
> Failing adding to v4l2_mbus_framefmt, I agree.
> 
> I notice also that there is only set_frame_desc and get_frame_desc, and
> no try_frame_desc.
> 
> In the only bridge driver that currently uses this interface,
> fimc-capture, when the user calls VIDIOC_TRY_FMT, then this is
> translated to a call to subdev.set_frame_desc.  Isn't this wrong?  I
> thought that TRY_* was never meant to modify the actual hardware, but
> only fill out the passed structure with what the device would be able
> to do, so don't we need also try_frame_desc?

I got the very same impression. Sylwester? :-)

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

Re: width and height of JPEG compressed images

[Sylwester Nawrocki]

Hi Sakari,

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:
>>> Hello,
>>>
>>> I am writing a driver for the sensor MT9D131.  This device supports
>>> digital zoom and JPEG compression.
>>>
>>> Although I am writing it for my company's internal purposes, it will
>>> be made open-source, so I would like to keep the API as portable as
>>> possible.
>>>
>>> The hardware reads AxB sensor pixels from its array, resamples them
>>> to CxD image pixels, and then compresses them to ExF bytes.
>>>
>>> The subdevice driver sets size AxB to the value it receives from
>>> v4l2_subdev_video_ops.s_crop().
>>>
>>> To enable compression then v4l2_subdev_video_ops.s_mbus_fmt() is
>>> called with fmt->code=V4L2_MBUS_FMT_JPEG_1X8.
>>>
>>> fmt->width and fmt->height then ought to specify the size of the
>>> compressed image ExF, that is, the size specified is the size in the
>>> format specified (the number of JPEG_1X8), not the size it would be
>>> in a raw format.
>>
>> In VIDIOC_S_FMT 'sizeimage' specifies size of the buffer for the
>> compressed frame at the bridge driver side. And width/height should
>> specify size of the re-sampled (binning, skipping ?) frame - CxD,
>> if I understand what  you are saying correctly.
>>
>> I don't quite what transformation is done at CxD ->  ExF. Why you are
>> using ExF (two numbers) to specify number of bytes ? And how can you
>> know exactly beforehand what is the frame size after compression ?
>> Does the sensor transmit fixed number of bytes per frame, by adding
>> some padding bytes if required to the compressed frame data ?
>>
>> Is it something like:
>>
>> sensor matrix (AxB pixels) ->  binning/skipping (CxD pixels) ->
>> ->  JPEG compresion (width = C, height = D, sizeimage ExF bytes)
>>
>> ?
>>> This allows the bridge driver to be compression agnostic.  It gets
>>> told how many bytes to allocate per buffer and it reads that many
>>> bytes.  It doesn't have to understand that the number of bytes isn't
>>> directly related to the number of pixels.
>>>
>>> So how does the user tell the driver what size image to capture
>>> before compression, CxD?
>>
>> I think you should use VIDIOC_S_FMT(width = C, height = D, sizeimage = ExF)
>> for that. And s_frame_desc sudev op could be used to pass sizeimage to the
>> sensor subdev driver.
>
> Agreed. Let me take this into account in the next RFC.

Thanks.

>>> (or alternatively, if you disagree and think CxD should be specified
>>> by s_fmt(), then how does the user specify ExF?)
>
> Does the user need to specify ExF, for other purposes than limiting the size
> of the image? I would leave this up to the sensor driver (with reasonable
> alignment). The sensor driver would tell about this to the receiver through

AFAIU ExF is closely related to the memory buffer size, so the sensor driver
itself wouldn't have enough information to fix up ExF, would it ?

> frame descriptors. (But still I don't think frame descriptors should be
> settable; what sensors can support is fully sensor specific and the
> parameters that typically need to be changed are quite limited in numbers.
> So I'd go with e.g. controls, again.)

I agree it would have been much more clear to have read only frame 
descriptors
outside of the subdev. But the issue with controls is that it would have
been difficult to define same parameter for multiple logical stream on the
data bus. And data interleaving is a standard feature, it is well 
defined in
the MIPI CSI-2 specification.

So my feeling is that we would be better off with data structure and
a callback, rather than creating multiple strange controls.

However if we don't use media bus format callbacks, nor frame descriptor
callbacks, then what ?... :) It sounds reasonable to me to have frame
frame descriptor defined by the sensor (data source) based on media bus
format, frame interval, link frequency, etc. Problematic seem to be
parameters that are now handled on the video node side, like, e.g. buffer
size.

--
Regards,
Sylwester

RE: width and height of JPEG compressed images

[Thomas Vajzovic]

Hello,

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.
>>>
>>> I think you should use VIDIOC_S_FMT(width = C, height = D, sizeimage
>>> = ExF) for that. And s_frame_desc sudev op could be used to pass
>>> sizeimage to the sensor subdev driver.
>>
>> Agreed. Let me take this into account in the next RFC.


I agree that in my use case the user only needs to be able to specify
sizeimage, and then be told in response what the adjusted value of
sizeimage is.


>> Does the user need to specify ExF, for other purposes than limiting
>> the size of the image? I would leave this up to the sensor driver
>> (with reasonable alignment). The sensor driver would tell about this
>> to the receiver through
>
> AFAIU ExF is closely related to the memory buffer size, so the sensor
> driver itself wouldn't have enough information to fix up ExF, would
>  it ?


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.

Eg: if the user requests images of 8K, then the bridge needs to know
that they will come out as 10 lines of 800 bytes.

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.

Eg: if the user requests sizeimage = 1601, then the sensor cannot
provide 1601x1 (width exceeds internal FIFO), it will have to tell
the bridge that it will give 800x2 or 801x2.  The userspace needs to
be told that sizeimage was adjusted to 1600 or 1602 because there are
data fields aligned to the end of the data.

(BTW, would you suggest rounding up or down in this case? If the user
knew how much memory that an embedded system had available and
specified sizeimage to the maximum, then rounding up might result in
failed allocation.  But then, if the user knows how much entropy-coded
JPEG data to expect, then rounding down might result in truncated
frames that have to be dropped.)


>> frame descriptors. (But still I don't think frame descriptors should
>> be settable; what sensors can support is fully sensor specific and the
>> parameters that typically need to be changed are quite limited in numbers.
>> So I'd go with e.g. controls, again.)
>
> I agree it would have been much more clear to have read only frame
> descriptors outside of the subdev. But the issue with controls is that
> it would have been difficult to define same parameter for multiple
> logical stream on the data bus. And data interleaving is a standard
> feature, it is well defined in the MIPI CSI-2 specification.

> So my feeling is that we would be better off with data structure and a
> callback, rather than creating multiple strange controls.

> However if we don't use media bus format callbacks, nor frame descriptor
> callbacks, then what ?... :) It sounds reasonable to me to have frame
> frame descriptor defined by the sensor (data source) based on media bus
> format, frame interval, link frequency, etc. Problematic seem to be
> parameters that are now handled on the video node side, like, e.g.
> buffer size.

I think that this is definitely not a candidate for using controls.
I think that whatever mechanism is used for setting sizemage on
JPEG sensors with 1D DMA, then the same mechanism needs to be extended
for this case.  Currently this is frame descriptors.

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.

Regards,
Tom

--
Mr T. Vajzovic
Software Engineer
Infrared Integrated Systems Ltd
Visit us at www.irisys.co.uk
Disclaimer: This e-mail message is confidential and for use by the addressee only. If the message is received by anyone other than the addressee, please return the message to the sender by replying to it and then delete the original message and the sent message from your computer. Infrared Integrated Systems Limited Park Circle Tithe Barn Way Swan Valley Northampton NN4 9BG Registration Number: 3186364.

Re: width and height of JPEG compressed images

[Sylwester Nawrocki]

Hi Thomas,

On 07/15/2013 11:18 AM, Thomas Vajzovic wrote:
> On 10 July 2013 20:44 Sylwester Nawrocki wrote:
>> On 07/07/2013 10:18 AM, Thomas Vajzovic wrote:
>>> On 06 July 2013 20:58 Sylwester Nawrocki wrote:
>>>> On 07/05/2013 10:22 AM, Thomas Vajzovic wrote:
>>>>>
>>>>> I am writing a driver for the sensor MT9D131.  This device supports
>>>>> digital zoom and JPEG compression.
>>>>>
>>>>> The hardware reads AxB sensor pixels from its array, resamples them
>>>>> to CxD image pixels, and then compresses them to ExF bytes.
>>>
>>> Yes you are correct that the sensor zero pads the compressed data to a
>>> fixed size.  That size must be specified in two separate registers,
>>> called spoof width and spoof height.  Above CxD is the image size
>>> after binning/skipping and resizing, ExF is the spoof size.
>>>
>>> The reason for two numbers for the number of bytes is that as the
>>> sensor outputs the JPEG bytes the VSYNC and HSYNC lines behave as
>>> though they were still outputting a 2D image with 8bpp.  This means
>>> that no changes are required in the bridge hardware.  I am trying to
>>> make it so very few changes are required in the bridge driver too.
>>> As far as the bridge driver is concerned the only size is ExF, it is
>>> unconcerned with CxD.
>>>
>>> v4l2_pix_format.width        = C;
>>> v4l2_pix_format.height       = D;
>>> v4l2_pix_format.bytesperline = E;
>>> v4l2_pix_format.sizeimage    = (E * F);
>>>
>>> bytesperline<  width
>>> (sizeimage % bytesperline) == 0
>>> (sizeimage / bytesperline)<  height
>>
>> bytesperline has not much meaning for compressed formats at the video
>> device (DMA) driver side.
>
> This is not true for the platform I am using.
>
> The Blackfin has a 2D DMA peripheral, meaning that it does need to
> separately know bytesperline and (sizeimage / bytesperline).

All right. We need to make it clear that the format on video node
refers to data in memory, while media bus format/frame descriptor
specifies how data is transmitted on the physical bus. When there
is scaling, etc. involved on the bridge side relations between the
two are not that straightforward. sizeimage / bytesperline needs to
be translatable to frame descriptor/media bus format information
and the other way around.

> These values have a physical hardware meaning in terms of the signals
> on the sync lines even though they do not have a logical meaning
> because the data is compressed.

Sure, what I'm trying to say is that V4L2 API [1] specifies bytesperline
as
     "Distance in bytes between the leftmost pixels in two adjacent lines".

This is what bytesperline currently means for the user applications.
While sizeimage is defined as:

     "Size in bytes of the buffer to hold a complete image, set by the
     driver. Usually this is bytesperline times height. When the image
     consists of variable length compressed data this is the maximum
     number of bytes required to hold an image."

User space may set sizeimage to an incorrect value and it must be adjusted
to some sane value by the kernel. The capture DMA engine driver need to
be able to query size of the image memory buffer at the sensor driver,
taking into account what the user provided sizeimage was. And it's the
sensor driver that will likely only have sufficient information to
determine buffer size, based on image width, height, etc.

>> For compressed streams like JPEG size of the memory buffer to
>> allocate is normally determined by sizeimage.
>
> It is two numbers in my use case.
>
>> 'bytesperline' could be less than 'width', that means a "virtual"
>> bits-per-pixel factor is less than 8. But this factor could (should?)
>> be configurable e.g. indirectly through V4L2_CID_JPEG_QUALITY control,
>
> This is absolutely not a "virtual" width, it is a real physical property
> of the hardware signal.  The hardware signal always has exactly 8 bits
> per sample, but its height and width (ExF) are not related to the image
> height and width (CxD).
>
> It is not appropriate to group the hardware data size together with
> compression controls for two reasons:
>
> Firstly, the bridge driver would need to intercept the control and then
> pass it on to the bridge driver because they both need to know E and F.

I assume you meant "pass it on to the sensor driver".

What I meant was only that compression parameters have some influence
on the resulting image data size. There is no need to refer directly
to any control on the bridge side, as long as the bridge can get required
information by some other API.

> Secondly, the pair of numbers (E,F) in my case have exaclty the same
> meaning and are used in exactly the same way as the single number
> (sizeimage) which is used in the cameras that use the current API.
> Logically the two numbers should be passed around and set and modified
> in all the same places that sizeimage currently is, but as a tuple.
> The two cannot be separated with one set using one API and the other
> a different API.

Sure, we just need to think of how to express (E, F) in the frame
descriptors API and teach the bridge driver to use it. As Sakari
mentioned width, height and bpp is probably going to be sufficient.

>> and the bridge can query it from the sensor through g_frame_desc subdev
>> op.  The bridge has normally no clue what the compression ratio at the
>> sensor side is.  It could hard code some default "bpp", but then it
>> needs to be ensured the sensor doesn't transmit more data than the size
>> of allocated buffer.
>
> It has no idea what the true compression ratio size is, but it does have
> to know the padded size.  The sensor will always send exactly that size.
>
>>> But the question now is how does the bridge device communicate this to
>>> the I2C subdevice?  v4l2_mbus_framefmt doesn't have bytesperline or
>>> sizeimage, and v4l2_mbus_frame_desc_entry has only length (which I
>>> presume is sizeimage) but not both dimensions.
>>
>> That's a good question. The frame descriptors really need more discussion
>> and improvement, to also cover use cases as your JPEG sensor.
>> Currently it is pretty pre-eliminary stuff, used by just a few drivers.
>> Here is the original RFC from Sakari [1].
>
> The version that has made it to kernel.org is much watered down from this
> proposal.  It could be suitable for doing what I need if an extra member
> were added, or preferably there should be something like:
>
> enum
> {
>    DMA_1D,
>    DMA_2D,
> };
>
> union {
>    struct {  // Valid if DMA_1D
>      u32 size;
>    };
>    struct {  // Valid if DMA_2D
>      u32 width;
>      u32 height;
>    };
> };

I guess size could be computed from width, height and additional bpp field,
when DMA doesn't care much about HSYNC/VSYNC layout. Then we could get rid
of size (length), only bpp/pixelcode relation would need to be clarified.

>> Since we can't add bytesperline/sizeimage to struct v4l2_mbus_framefmt
>
> Isn't this a sensible case for using some of those reserved bytes?

I tried to add framesamples field to this data structure [2], but it wasn't
well received. The downside of such additions is that user space gets 
involved
in passing those additional parameters between subdevs like image sensor,
MIPI CIS-2 receiver and others.

In some cases one would want to add horizontal/vertical blanking 
information,
pixel clock frequency or other parameters that are best handled internally
in the kernel.

Your proposal above sounds sane, I've seen already 1D/2D DMA notations
in some documentation. Is datasheet of your bridge device available
publicly ? Which Blackfin processor is that ?

> If not, why are they there?
>
>> I think struct v4l2_mbus_frame_desc_entry needs to be extended and
>> interaction between subdev ops like video.{s,g}_mbus_fmt,
>> pad.{set,get}_fmt needs to be specified.
>
> Failing adding to v4l2_mbus_framefmt, I agree.

There are reasons for not adding such hardware specific information to
this user visible data structure. Let's leave it alone and try to explore
other possibilities.

> I notice also that there is only set_frame_desc and get_frame_desc, and
> no try_frame_desc.
>
> In the only bridge driver that currently uses this interface,
> fimc-capture, when the user calls VIDIOC_TRY_FMT, then this is
> translated to a call to subdev.set_frame_desc.  Isn't this wrong?  I
> thought that TRY_* was never meant to modify the actual hardware, but
> only fill out the passed structure with what the device would be able
> to do, so don't we need also try_frame_desc?

Yes, you're right. set_frame_desc should never be called from
VIDIOC_TRY_FMT. try_frame_desc could be added and we probably need
to specify some sort of priority which operation (s_frame_desc, set_fmt)
is allowed to freely modify which parameters and what parts of the
frame descriptor data structure should be adjusted to currently set
parameters by some other (user) API.

That might not sound like an ideal solution, but we now have a situation
where some parameters needs to be configured in the kernel while others
are configurable by user, and all those parameters are more or less
dependant on each other.

--
Regards,
Sylwester

[1] http://linuxtv.org/downloads/v4l-dvb-apis/pixfmt.html#v4l2-pix-format
[2] http://www.spinics.net/lists/linux-media/msg41788.html

RE: width and height of JPEG compressed images

[Thomas Vajzovic]

Hi Sylwester,

On 22 July 2013 22:48 Sylwester Nawrocki wrote:
> On 07/15/2013 11:18 AM, Thomas Vajzovic wrote:
>> On 10 July 2013 20:44 Sylwester Nawrocki wrote:
>>> On 07/07/2013 10:18 AM, Thomas Vajzovic wrote:
>>>> On 06 July 2013 20:58 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.
>>>>
>>>> Yes you are correct that the sensor zero pads the compressed data to
>>>> a fixed size.  That size must be specified in two separate
>>>> registers, called spoof width and spoof height.  Above CxD is the
>>>> image size after binning/skipping and resizing, ExF is the spoof size.
>
> All right. We need to make it clear that the format on video node
> refers to data in memory, while media bus format/frame descriptor
> specifies how data is transmitted on the physical bus. When there is
> scaling, etc. involved on the bridge side relations between the two
> are not that straightforward. sizeimage / bytesperline needs to be
> translatable to frame descriptor/media bus format information and the
> other way around.

I'm not sure that translating them is reasonable.  The image width and
height are one thing, and the data size (whether 1D or 2D) is another
thing.  They just need to be expressed explicitly.

>> Secondly, the pair of numbers (E,F) in my case have exaclty the same
>> meaning and are used in exactly the same way as the single number
>> (sizeimage) which is used in the cameras that use the current API.
>> Logically the two numbers should be passed around and set and modified
>> in all the same places that sizeimage currently is, but as a tuple.
>> The two cannot be separated with one set using one API and the other a
>> different API.
>
> Sure, we just need to think of how to express (E, F) in the frame
> descriptors API and teach the bridge driver to use it. As Sakari
> mentioned width, height and bpp is probably going to be sufficient.

Bits-per-image-pixel is variable, but I assume you mean "average-
bits-per-image-pixel".  This is confusing and inexact:  What if the
user wants to compress 800x600 to 142kB? then bpp = 2.4234667.
This number doesn't really mean very much, and how would you express
it so that the bridge always get exact pair of integers that the sensor
chose without rounding error?  I suggest that the clean and sensible
solution is to explicitly express physical width, with physical-bits-
per-pixel = always 8 (assuming FMT_JPEG_1X8).

Many thanks for your time on this.  Please see also my reply at
Mon 22/07/2013 09:41.

> Your proposal above sounds sane, I've seen already 1D/2D DMA notations
> in some documentation. Is datasheet of your bridge device available
> publicly ? Which Blackfin processor is that ?

http://www.analog.com/static/imported-files/processor_manuals/ADSP-BF51x_hwr_rev1.2.pdf

Best regards,
Tom

--
Mr T. Vajzovic
Software Engineer
Infrared Integrated Systems Ltd
Visit us at www.irisys.co.uk

Disclaimer: This e-mail message is confidential and for use by the addressee only. If the message is received by anyone other than the addressee, please return the message to the sender by replying to it and then delete the original message and the sent message from your computer. Infrared Integrated Systems Limited Park Circle Tithe Barn Way Swan Valley Northampton NN4 9BG Registration Number: 3186364.

Re: width and height of JPEG compressed images

[Sakari Ailus]

Hi Sylwester,

On Sun, Jul 21, 2013 at 10:38:18PM +0200, 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:
> >>>Hello,
> >>>
> >>>I am writing a driver for the sensor MT9D131.  This device supports
> >>>digital zoom and JPEG compression.
> >>>
> >>>Although I am writing it for my company's internal purposes, it will
> >>>be made open-source, so I would like to keep the API as portable as
> >>>possible.
> >>>
> >>>The hardware reads AxB sensor pixels from its array, resamples them
> >>>to CxD image pixels, and then compresses them to ExF bytes.
> >>>
> >>>The subdevice driver sets size AxB to the value it receives from
> >>>v4l2_subdev_video_ops.s_crop().
> >>>
> >>>To enable compression then v4l2_subdev_video_ops.s_mbus_fmt() is
> >>>called with fmt->code=V4L2_MBUS_FMT_JPEG_1X8.
> >>>
> >>>fmt->width and fmt->height then ought to specify the size of the
> >>>compressed image ExF, that is, the size specified is the size in the
> >>>format specified (the number of JPEG_1X8), not the size it would be
> >>>in a raw format.
> >>
> >>In VIDIOC_S_FMT 'sizeimage' specifies size of the buffer for the
> >>compressed frame at the bridge driver side. And width/height should
> >>specify size of the re-sampled (binning, skipping ?) frame - CxD,
> >>if I understand what  you are saying correctly.
> >>
> >>I don't quite what transformation is done at CxD ->  ExF. Why you are
> >>using ExF (two numbers) to specify number of bytes ? And how can you
> >>know exactly beforehand what is the frame size after compression ?
> >>Does the sensor transmit fixed number of bytes per frame, by adding
> >>some padding bytes if required to the compressed frame data ?
> >>
> >>Is it something like:
> >>
> >>sensor matrix (AxB pixels) ->  binning/skipping (CxD pixels) ->
> >>->  JPEG compresion (width = C, height = D, sizeimage ExF bytes)
> >>
> >>?
> >>>This allows the bridge driver to be compression agnostic.  It gets
> >>>told how many bytes to allocate per buffer and it reads that many
> >>>bytes.  It doesn't have to understand that the number of bytes isn't
> >>>directly related to the number of pixels.
> >>>
> >>>So how does the user tell the driver what size image to capture
> >>>before compression, CxD?
> >>
> >>I think you should use VIDIOC_S_FMT(width = C, height = D, sizeimage = ExF)
> >>for that. And s_frame_desc sudev op could be used to pass sizeimage to the
> >>sensor subdev driver.
> >
> >Agreed. Let me take this into account in the next RFC.
> 
> Thanks.
> 
> >>>(or alternatively, if you disagree and think CxD should be specified
> >>>by s_fmt(), then how does the user specify ExF?)
> >
> >Does the user need to specify ExF, for other purposes than limiting the size
> >of the image? I would leave this up to the sensor driver (with reasonable
> >alignment). The sensor driver would tell about this to the receiver through
> 
> AFAIU ExF is closely related to the memory buffer size, so the sensor driver
> itself wouldn't have enough information to fix up ExF, would it ?

If the desired sizeimage is known, F can be calculated if E is fixed, say
1024 should probably work for everyone, shoulnd't it?

> >frame descriptors. (But still I don't think frame descriptors should be
> >settable; what sensors can support is fully sensor specific and the
> >parameters that typically need to be changed are quite limited in numbers.
> >So I'd go with e.g. controls, again.)
> 
> I agree it would have been much more clear to have read only frame
> descriptors
> outside of the subdev. But the issue with controls is that it would have
> been difficult to define same parameter for multiple logical stream on the
> data bus. And data interleaving is a standard feature, it is well
> defined in
> the MIPI CSI-2 specification.
> 
> So my feeling is that we would be better off with data structure and
> a callback, rather than creating multiple strange controls.

That's true for controls. I'd hope that we could connect properties (or
extended^2 controls or whatever) to arbitrary attributes vs.
subdevs or V4L2 devices currently. But we'd need the the properties first in
that case.

In the meantime I'd be fine with even a few funnily named controls.

> However if we don't use media bus format callbacks, nor frame descriptor
> callbacks, then what ?... :) It sounds reasonable to me to have frame
> frame descriptor defined by the sensor (data source) based on media bus
> format, frame interval, link frequency, etc. Problematic seem to be
> parameters that are now handled on the video node side, like, e.g. buffer
> size.

Is that really problematic?

If my memory serves me right, passing the image size in frame descriptor was
done for the reason that controls are associated with a device rather than a
more fine grained object and a bridge driver didn't have a good way to tell
the sensor driver its control x was since unchangeable.

Resolving the two would make it possible to meaningfully use controls for
the purpose as far as I understand. (The second is easy: make
v4l2_ctrl_grab() use integer rather than a single bit. Possibly ensure the
coupl:e of users are ok with it, too.)

-- 
Cheers,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

RE: width and height of JPEG compressed images

[Thomas Vajzovic]

Hi Sakiri,

 On 23 July 2013 23:21 Sakari Ailus wrote:
> On Sun, Jul 21, 2013 at 10:38:18PM +0200, 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.
>>>>>
>>>> sensor matrix (AxB pixels) ->  binning/skipping (CxD pixels) ->
>>>> ->  JPEG compresion (width = C, height = D, sizeimage ExF bytes)
>>>
>>> Does the user need to specify ExF, for other purposes than limiting
>>> the size of the image? I would leave this up to the sensor driver
>>> (with reasonable alignment). The sensor driver would tell about this
>>> to the receiver through
>>
>> AFAIU ExF is closely related to the memory buffer size, so the sensor
>> driver itself wouldn't have enough information to fix up ExF, would it ?
>
> If the desired sizeimage is known, F can be calculated if E is fixed, say
> 1024 should probably work for everyone, shoulnd't it?

It's a nice clean idea (and I did already consider it) but it reduces the
flexibility of the system as a whole.

Suppose an embedded device wants to send the compressed image over a
network in packets of 1500 bytes, and they want to allow 3 packets per
frame.  Your proposal limits sizeimage to a multiple of 1K, so they have
to set sizeimage to 4K when they want 4.5K, meaning that they waste 500
bytes of bandwidth every frame.

You could say "tough luck, extra overhead like this is something you should
expect if you want to use a general purpose API like V4L2", but why make
it worse if we can make it better?

Best regards,
Tom

--
Mr T. Vajzovic
Software Engineer
Infrared Integrated Systems Ltd
Visit us at www.irisys.co.uk
Disclaimer: This e-mail message is confidential and for use by the addressee only. If the message is received by anyone other than the addressee, please return the message to the sender by replying to it and then delete the original message and the sent message from your computer. Infrared Integrated Systems Limited Park Circle Tithe Barn Way Swan Valley Northampton NN4 9BG Registration Number: 3186364.

Re: width and height of JPEG compressed images

[Sylwester Nawrocki]

Hi,

On 07/24/2013 09:47 AM, Thomas Vajzovic wrote:
>  On 23 July 2013 23:21 Sakari Ailus wrote:
>> On Sun, Jul 21, 2013 at 10:38:18PM +0200, 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.
>>>>>>
>>>>> sensor matrix (AxB pixels) ->  binning/skipping (CxD pixels) ->
>>>>> ->  JPEG compresion (width = C, height = D, sizeimage ExF bytes)
>>>>
>>>> Does the user need to specify ExF, for other purposes than limiting
>>>> the size of the image? I would leave this up to the sensor driver
>>>> (with reasonable alignment). The sensor driver would tell about this
>>>> to the receiver through
>>>
>>> AFAIU ExF is closely related to the memory buffer size, so the sensor
>>> driver itself wouldn't have enough information to fix up ExF, would it ?
>>
>> If the desired sizeimage is known, F can be calculated if E is fixed, say
>> 1024 should probably work for everyone, shoulnd't it?
> 
> It's a nice clean idea (and I did already consider it) but it reduces the
> flexibility of the system as a whole.
> 
> Suppose an embedded device wants to send the compressed image over a
> network in packets of 1500 bytes, and they want to allow 3 packets per
> frame.  Your proposal limits sizeimage to a multiple of 1K, so they have
> to set sizeimage to 4K when they want 4.5K, meaning that they waste 500
> bytes of bandwidth every frame.
> 
> You could say "tough luck, extra overhead like this is something you should
> expect if you want to use a general purpose API like V4L2", but why make
> it worse if we can make it better?

I entirely agree with that. Other issue with fixed number of samples
per line is that internal (FIFO) line buffer size of the transmitter
devices will vary, and for example some devices might have line buffer
smaller than the value we have arbitrarily chosen. I'd expect the
optimal number of samples per line to vary among different devices
and use cases.


Regards,
Sylwester

Re: width and height of JPEG compressed images

[Sylwester Nawrocki]

Hi Thomas,

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.
>>>>
>>>> I think you should use VIDIOC_S_FMT(width = C, height = D, sizeimage
>>>> = ExF) for that. And s_frame_desc sudev op could be used to pass
>>>> sizeimage to the sensor subdev driver.
>>>
>>> Agreed. Let me take this into account in the next RFC.
> 
> 
> I agree that in my use case the user only needs to be able to specify
> sizeimage, and then be told in response what the adjusted value of
> sizeimage is.
> 
> 
>>> Does the user need to specify ExF, for other purposes than limiting
>>> the size of the image? I would leave this up to the sensor driver
>>> (with reasonable alignment). The sensor driver would tell about this
>>> to the receiver through
>>
>> AFAIU ExF is closely related to the memory buffer size, so the sensor
>> driver itself wouldn't have enough information to fix up ExF, would
>>  it ?
> 
> 
> 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.
> 
> Eg: if the user requests images of 8K, then the bridge needs to know
> that they will come out as 10 lines of 800 bytes.
> 
> 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.
> 
> Eg: if the user requests sizeimage = 1601, then the sensor cannot
> provide 1601x1 (width exceeds internal FIFO), it will have to tell
> the bridge that it will give 800x2 or 801x2.  The userspace needs to
> be told that sizeimage was adjusted to 1600 or 1602 because there are
> data fields aligned to the end of the data.

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.

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.

> (BTW, would you suggest rounding up or down in this case? If the user
> knew how much memory that an embedded system had available and
> specified sizeimage to the maximum, then rounding up might result in
> failed allocation.  But then, if the user knows how much entropy-coded
> JPEG data to expect, then rounding down might result in truncated
> frames that have to be dropped.)

I think the sensor should always round up, the bridge can then apply
any upper limits. I wouldn't rely too much on what sizeimage user
space provides in VIDIOC_S_FMT.

>>> frame descriptors. (But still I don't think frame descriptors should
>>> be settable; what sensors can support is fully sensor specific and the
>>> parameters that typically need to be changed are quite limited in numbers.
>>> So I'd go with e.g. controls, again.)
>>
>> I agree it would have been much more clear to have read only frame
>> descriptors outside of the subdev. But the issue with controls is that
>> it would have been difficult to define same parameter for multiple
>> logical stream on the data bus. And data interleaving is a standard
>> feature, it is well defined in the MIPI CSI-2 specification.
> 
>> So my feeling is that we would be better off with data structure and a
>> callback, rather than creating multiple strange controls.
> 
>> However if we don't use media bus format callbacks, nor frame descriptor
>> callbacks, then what ?... :) It sounds reasonable to me to have frame
>> frame descriptor defined by the sensor (data source) based on media bus
>> format, frame interval, link frequency, etc. Problematic seem to be
>> parameters that are now handled on the video node side, like, e.g.
>> buffer size.
> 
> I think that this is definitely not a candidate for using controls.
> I think that whatever mechanism is used for setting sizemage on
> JPEG sensors with 1D DMA, then the same mechanism needs to be extended
> for this case.  Currently this is frame descriptors.
> 
> 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.

--
Regards,
Sylwester

Re: width and height of JPEG compressed images

[Sakari Ailus]

Hi Sylwester and Thomas,

On Wed, Jul 24, 2013 at 10:39:11AM +0200, Sylwester Nawrocki wrote:
> Hi,
> 
> On 07/24/2013 09:47 AM, Thomas Vajzovic wrote:
> >  On 23 July 2013 23:21 Sakari Ailus wrote:
> >> On Sun, Jul 21, 2013 at 10:38:18PM +0200, 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.
> >>>>>>
> >>>>> sensor matrix (AxB pixels) ->  binning/skipping (CxD pixels) ->
> >>>>> ->  JPEG compresion (width = C, height = D, sizeimage ExF bytes)
> >>>>
> >>>> Does the user need to specify ExF, for other purposes than limiting
> >>>> the size of the image? I would leave this up to the sensor driver
> >>>> (with reasonable alignment). The sensor driver would tell about this
> >>>> to the receiver through
> >>>
> >>> AFAIU ExF is closely related to the memory buffer size, so the sensor
> >>> driver itself wouldn't have enough information to fix up ExF, would it ?
> >>
> >> If the desired sizeimage is known, F can be calculated if E is fixed, say
> >> 1024 should probably work for everyone, shoulnd't it?
> > 
> > It's a nice clean idea (and I did already consider it) but it reduces the
> > flexibility of the system as a whole.
> > 
> > Suppose an embedded device wants to send the compressed image over a
> > network in packets of 1500 bytes, and they want to allow 3 packets per
> > frame.  Your proposal limits sizeimage to a multiple of 1K, so they have
> > to set sizeimage to 4K when they want 4.5K, meaning that they waste 500
> > bytes of bandwidth every frame.
> > 
> > You could say "tough luck, extra overhead like this is something you should
> > expect if you want to use a general purpose API like V4L2", but why make
> > it worse if we can make it better?
> 
> I entirely agree with that. Other issue with fixed number of samples
> per line is that internal (FIFO) line buffer size of the transmitter
> devices will vary, and for example some devices might have line buffer
> smaller than the value we have arbitrarily chosen. I'd expect the
> optimal number of samples per line to vary among different devices
> and use cases.

I guess the sensor driver could factor the size as well (provided it can
choose an arbitrary size) but then to be fully generic, I think alignment
must also be taken care of. Many receivers might require width to be even
but some might have tighter requirements. They have a minimum width, too.

To make this working in a generic case might not be worth the time and
effort of being able to shave up to 1 kiB off of video buffer allocations.

Remember v4l2_buffer.length is different from v4l2_pix_format.sizeimage.
Hmm. Yes --- so to the sensor goes desired maximum size, and back you'd get
ExF (i.e. buffer length) AND the size of the image.

What do you think?

-- 
Cheers,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

RE: width and height of JPEG compressed images

[Thomas Vajzovic]

Hi,

On 26 July 2013 10:07 Sakari Ailus wrote:
> On Wed, Jul 24, 2013 at 10:39:11AM +0200, Sylwester Nawrocki wrote:
>> On 07/24/2013 09:47 AM, Thomas Vajzovic wrote:
>>> On 23 July 2013 23:21 Sakari Ailus wrote:
>>>> On Sun, Jul 21, 2013 at 10:38:18PM +0200, 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.
>>>>>>>>
>>>>>>> sensor matrix (AxB pixels) ->  binning/skipping (CxD pixels) ->
>>>>>>> ->  JPEG compresion (width = C, height = D, sizeimage ExF bytes)
>>>>>>
>>>>>> Does the user need to specify ExF, for other purposes than
>>>>>> limiting the size of the image? I would leave this up to the
>>>>>> sensor driver (with reasonable alignment). The sensor driver
>>>>>> would tell about this to the receiver through
>>>>>
>>>>> AFAIU ExF is closely related to the memory buffer size, so the
>>>>> sensor driver itself wouldn't have enough information to fix up ExF, would it ?
>>>>
>>>> If the desired sizeimage is known, F can be calculated if E is
>>>> fixed, say
>>>> 1024 should probably work for everyone, shoulnd't it?
>>>
>>> It's a nice clean idea (and I did already consider it) but it
>>> reduces the flexibility of the system as a whole.
>>>
>>> Suppose an embedded device wants to send the compressed image over a
>>> network in packets of 1500 bytes, and they want to allow 3 packets
>>> per frame.  Your proposal limits sizeimage to a multiple of 1K, so
>>> they have to set sizeimage to 4K when they want 4.5K, meaning that
>>> they waste 500 bytes of bandwidth every frame.
>>>
>>> You could say "tough luck, extra overhead like this is something you
>>> should expect if you want to use a general purpose API like V4L2",
>>> but why make it worse if we can make it better?
>>
>> I entirely agree with that. Other issue with fixed number of samples
>> per line is that internal (FIFO) line buffer size of the transmitter
>> devices will vary, and for example some devices might have line buffer
>> smaller than the value we have arbitrarily chosen. I'd expect the
>> optimal number of samples per line to vary among different devices and
>> use cases.
>
> I guess the sensor driver could factor the size as well (provided it
> can choose an arbitrary size) but then to be fully generic, I think
> alignment must also be taken care of. Many receivers might require
> width to be even but some might have tighter requirements. They have
> a minimum width, too.

> To make this working in a generic case might not be worth the time
> and effort of being able to shave up to 1 kiB off of video buffer
> allocations.

I think that a good enough solution here is that the code within each
sensor driver that does the factorization has to be written to account
for whatever reasonable restrictions that a bridge might require.

Eg: if the userspace requests 49 bytes, it doesn't give back 7x7,
because it knows that some bridges don't like odd numbers.

A sensor driver author would have to do a quick survey of bridges to
see what was likely to be problematic.  A bit of common sense would
solve the vast majority of cases.  After that if the bridge didn't
like what the sensor set, then the whole operation would fail.  The
user would then have to make a feature request to the sensor driver
author saying "can you please tweak it to work with such-a-bridge".

This solution is only slightly more complicated than picking a fixed
width, and I think that the advantage is worth the extra complication.

> Remember v4l2_buffer.length is different from
>  v4l2_pix_format.sizeimage.
> Hmm. Yes --- so to the sensor goes desired maximum size, and back
> you'd get ExF (i.e. buffer length) AND the size of the image.

I really don't understand this last paragraph. Try adding coffee ;-)

Best regards,
Tom

--
Mr T. Vajzovic
Software Engineer
Infrared Integrated Systems Ltd
Visit us at www.irisys.co.uk
Disclaimer: This e-mail message is confidential and for use by the addressee only. If the message is received by anyone other than the addressee, please return the message to the sender by replying to it and then delete the original message and the sent message from your computer. Infrared Integrated Systems Limited Park Circle Tithe Barn Way Swan Valley Northampton NN4 9BG Registration Number: 3186364.

RE: width and height of JPEG compressed images

[Thomas Vajzovic]

Hi,

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).

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?

> 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.

>> 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.

Best regards,
Tom

--
Mr T. Vajzovic
Software Engineer
Infrared Integrated Systems Ltd
Visit us at www.irisys.co.uk
Disclaimer: This e-mail message is confidential and for use by the addressee only. If the message is received by anyone other than the addressee, please return the message to the sender by replying to it and then delete the original message and the sent message from your computer. Infrared Integrated Systems Limited Park Circle Tithe Barn Way Swan Valley Northampton NN4 9BG Registration Number: 3186364.

Re: width and height of JPEG compressed images

[Sakari Ailus]

Hi Tom,

Before replying the rest, let me first ask you a question. Does ExF define
the size of the image, or does it define its maximum size? I think that may
make a big difference here.

-- 
Cheers,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

RE: width and height of JPEG compressed images

[Thomas Vajzovic]

It defines the exact size of the physical frame.  The JPEG data is padded to this size.  The size of the JPEG before it was padded is also written into the last word of the physical frame.


________________________________________
From: Sakari Ailus [sakari.ailus@iki.fi]
Sent: 07 August 2013 10:35
To: Thomas Vajzovic
Cc: Sylwester Nawrocki; Sylwester Nawrocki; linux-media@vger.kernel.org; Laurent Pinchart
Subject: Re: width and height of JPEG compressed images

Hi Tom,

Before replying the rest, let me first ask you a question. Does ExF define
the size of the image, or does it define its maximum size? I think that may
make a big difference here.

--
Cheers,

Sakari Ailus
e-mail: sakari.ailus@iki.fi     XMPP: sailus@retiisi.org.uk
Disclaimer: This e-mail message is confidential and for use by the addressee only. If the message is received by anyone other than the addressee, please return the message to the sender by replying to it and then delete the original message and the sent message from your computer. Infrared Integrated Systems Limited Park Circle Tithe Barn Way Swan Valley Northampton NN4 9BG Registration Number: 3186364.

Re: width and height of JPEG compressed images

[Sakari Ailus]

Hi Thomas,

On Wed, Aug 07, 2013 at 05:43:56PM +0000, Thomas Vajzovic wrote:
> It defines the exact size of the physical frame.  The JPEG data is padded
> to this size. The size of the JPEG before it was padded is also written
> into the last word of the physical frame.

In that case, I think the issue is much lesser than we thought originally:
the image may still well be smaller than the buffer even if the buffer is
e.g. page aligned.

-- 
Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

Re: width and height of JPEG compressed images

[Laurent Pinchart]

Hi,

On Wednesday 21 August 2013 16:17:37 Sakari Ailus wrote:
> On Wed, Aug 07, 2013 at 05:43:56PM +0000, Thomas Vajzovic wrote:
> > It defines the exact size of the physical frame.  The JPEG data is padded
> > to this size. The size of the JPEG before it was padded is also written
> > into the last word of the physical frame.

That would require either using a custom pixel format and have userspace 
reading the size from the buffer, or mapping the buffer in kernel space and 
reading the size there. The latter is easier for userspace, but might it 
hinder performances ?

> In that case, I think the issue is much lesser than we thought originally:
> the image may still well be smaller than the buffer even if the buffer is
> e.g. page aligned.

-- 
Regards,

Laurent Pinchart

Re: width and height of JPEG compressed images

[Sakari Ailus]

Hi Thomas,

On Tue, Aug 06, 2013 at 04:26:56PM +0000, Thomas Vajzovic wrote:
> Hi,
> 
> 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:

Are there any bridge devices that CANNOT receive 2D images? I've never seen
any.

> 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).

And typically also 2D-only bridges have very limited maximum image width
which is unsuitable for any decent images. I'd rather like to only support
cases that we actually have right now.

> 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?
> 
> > 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

Pixelcode would be required to tell which other kind of data is produced by
the device. But I agree in principle --- there could (theoretically) be
multiple pixelcodes that you might want to configure on a sensor. We don't
have a way to express that currently.

I think we'd need one additional level of abstraction to express that; in
that case pads could have several media bus formats associated with them
(which we'd need to enumerate, too). One additional field to struct
v4l2_subdev_format might suffice.

I think frame descriptors would still be needed: there's information such as
the line number on which the particular piece of image begins etc.

> 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.
> 
> >> 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?

Do you have an example of something you'd like to set (or try) in frame
descriptors outside struct v4l2_subdev_format?

> 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.

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

RE: width and height of JPEG compressed images

[Thomas Vajzovic]

Hi,

On 21 August 2013 14:29, Laurent Pinchart wrote:
> On Wednesday 21 August 2013 16:17:37 Sakari Ailus wrote:
>> On Wed, Aug 07, 2013 at 05:43:56PM +0000, Thomas Vajzovic wrote:
>>> It defines the exact size of the physical frame.  The JPEG data is
>>> padded to this size. The size of the JPEG before it was padded is
>>> also written into the last word of the physical frame.

That would require either using a custom pixel format and have userspace
reading the size from the buffer, or mapping the buffer in kernel space
and reading the size there. The latter is easier for userspace, but
might it hinder performances ?

I think it ought to be a custom format and handled in userspace,
otherwise the bridge driver would have to call a subdev function
each frame to get it to fix-up the used size each time, which is
quite ugly.

Regards,
Tom

--
Mr T. Vajzovic
Software Engineer
Infrared Integrated Systems Ltd
Visit us at www.irisys.co.uk
Disclaimer: This e-mail message is confidential and for use by the addressee only. If the message is received by anyone other than the addressee, please return the message to the sender by replying to it and then delete the original message and the sent message from your computer. Infrared Integrated Systems Limited Park Circle Tithe Barn Way Swan Valley Northampton NN4 9BG Registration Number: 3186364.

RE: width and height of JPEG compressed images

[Thomas Vajzovic]

Hi,

On 21 August 2013 14:34, Sakari Ailus wrote:
> On Tue, Aug 06, 2013 at 04:26:56PM +0000, 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:
>
> Are there any bridge devices that CANNOT receive 2D images? I've
> never seen any.

I meant "bridge with 1D DMA".

>> 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).
>
> And typically also 2D-only bridges have very limited maximum image
> width which is unsuitable for any decent images. I'd rather like to
> only support cases that we actually have right now.

That makes sense.  I would make a small change though:

I think your proposed structure and protocol has redundant data
which could lead to ambiguity.

Perhaps the structure should only have size and samples_per_line.
If the subdev supports 2D output of a compressed stream then it examines
size, and sets samples_per_line and adjusts size.  If not then it
may still adjust size but leaves samples_per_line zeroed.  As you said
if the bridge finds samples_per_line still zeroed and it needs it then
it will have to give up.  If it has a non-zero samples_per_line then it
can divide to find num_lines.

>>> 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
>
> Pixelcode would be required to tell which other kind of data is
> produced by the device. But I agree in principle --- there could
> (theoretically) be multiple pixelcodes that you might want to
> configure on a sensor. We don't have a way to express that currently.

I wasn't thinking that set_frame_desc should be able to configure
currently unselected pixelcodes, quite the contrary, I would expect
that the pad should have a selected pixelcode, set by set_mbus_fmt,
so having pixelcode in frame_desc_entry is extra duplication, I don't
know why it is there.

> Do you have an example of something you'd like to set (or try) in frame
> descriptors outside struct v4l2_subdev_format?

I only have a need to try/set the buffersize which is tried/set by
userspace.


Best regards,
Tom

--
Mr T. Vajzovic
Software Engineer
Infrared Integrated Systems Ltd
Visit us at www.irisys.co.uk
Disclaimer: This e-mail message is confidential and for use by the addressee only. If the message is received by anyone other than the addressee, please return the message to the sender by replying to it and then delete the original message and the sent message from your computer. Infrared Integrated Systems Limited Park Circle Tithe Barn Way Swan Valley Northampton NN4 9BG Registration Number: 3186364.

Re: width and height of JPEG compressed images

[Sylwester Nawrocki]

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.

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,
Sylwester

Re: width and height of JPEG compressed images

[Laurent Pinchart]

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