pac7302-webcams and libv4lconvert interaction

pac7302-webcams and libv4lconvert interaction

[Frank Schäfer]

Hi,

I'm currently looking into the gspca_pac7302-driver and how it interacts
with libv4lconvert.
This is how it currently works
- driver announces v4l2_pix_format 640x480 (width x height)
- the frames (jpeg) passed to userspace are encoded as 480x640 and this
complies with the jpeg-header we generate
- libv4lconvert checks width/height in the jpeg-header and compares them
with the image format announced by the kernel:
   a) values are the same:
      1) V4LCONTROL_ROTATED_90_JPEG is NOT set for the device (standard
case):
          => everything is fine, image is decoded
      2) V4LCONTROL_ROTATED_90_JPEG is set for the device:
          => libv4lconvert bails out with -EIO displaying the error
message "unexpected width / height in JPEG header: expected: 640x480,
header: 480x640"
   b) values are different:
      1) V4LCONTROL_ROTATED_90_JPEG is NOT set:
          => libv4lconvert bails out with -EIO displaying the error
message "unexpected width / height in JPEG header: expected: 640x480,
header: 480x640"
      2) V4LCONTROL_ROTATED_90_JPEG is set:
          => image is decoded and rotated correctly


Thinking about this for some minutes:

1) shouldn't the kernel always announce the real image format (size) of
the data it passes to userspace ?
Current behavior seems inconsistent to me...
Announcing the actual image size allows applications which trust the API
value more than the value in the frame header to decode the image
correctly without using libv4lconvert (although the image would still be
rotated).

2) shouldn't libv4lconvert always rotate the image if
V4LCONTROL_ROTATED_90_JPEG is set for a device ?
It seems like a2) is a bug, because the expected size should be 640x480,
too.

3) because all pac7302 devices are sending rotated image data, we should
add them ALL to libv4lconvert. Currently only 4 of the 14 devices are on
the list.
Do you want me to send a patch ?


What do you think ?

Regards,
Frank

Re: pac7302-webcams and libv4lconvert interaction

[Hans de Goede]

Hi,

On 09/06/2012 05:13 PM, Frank Schäfer wrote:
>
> Hi,
>
> I'm currently looking into the gspca_pac7302-driver and how it interacts
> with libv4lconvert.
> This is how it currently works
> - driver announces v4l2_pix_format 640x480 (width x height)
> - the frames (jpeg) passed to userspace are encoded as 480x640 and this
> complies with the jpeg-header we generate
> - libv4lconvert checks width/height in the jpeg-header and compares them
> with the image format announced by the kernel:
>     a) values are the same:
>        1) V4LCONTROL_ROTATED_90_JPEG is NOT set for the device (standard
> case):
>            => everything is fine, image is decoded
>        2) V4LCONTROL_ROTATED_90_JPEG is set for the device:
>            => libv4lconvert bails out with -EIO displaying the error
> message "unexpected width / height in JPEG header: expected: 640x480,
> header: 480x640"
>     b) values are different:
>        1) V4LCONTROL_ROTATED_90_JPEG is NOT set:
>            => libv4lconvert bails out with -EIO displaying the error
> message "unexpected width / height in JPEG header: expected: 640x480,
> header: 480x640"
>        2) V4LCONTROL_ROTATED_90_JPEG is set:
>            => image is decoded and rotated correctly
>
>
> Thinking about this for some minutes:
>
> 1) shouldn't the kernel always announce the real image format (size) of
> the data it passes to userspace ?

It is passing the real size, the data is just in a vary funky format which
needs rotation as part of its "decoding" / decompression.

> Current behavior seems inconsistent to me...
> Announcing the actual image size allows applications which trust the API
> value more than the value in the frame header to decode the image
> correctly without using libv4lconvert (although the image would still be
> rotated).

That assumes that the app would know how to decompress the data which it
will not know, these cams do not generate standard JPEG data, libv4lconvert's
decompression code is the only decompression code for this fsck-ed up JPEG-s,
short of the windows drivers code.

> 2) shouldn't libv4lconvert always rotate the image if
> V4LCONTROL_ROTATED_90_JPEG is set for a device ?
> It seems like a2) is a bug, because the expected size should be 640x480,
> too.

rotating by 90 degrees also swaps the width and height, which are usually
not the same, so rotating an image which starts at 640x480 will yield
a final image of 480x640 which will not be what the app expects.


> 3) because all pac7302 devices are sending rotated image data, we should
> add them ALL to libv4lconvert. Currently only 4 of the 14 devices are on
> the list.
> Do you want me to send a patch ?

I see you've send a patch in the mean time, I'll reply in more detail to
this to the patch mail.

Regards,

Hans

Re: pac7302-webcams and libv4lconvert interaction

[Frank Schäfer]

Hi Hans,

Am 09.09.2012 23:20, schrieb Hans de Goede:
> Hi,
>
> On 09/06/2012 05:13 PM, Frank Schäfer wrote:
>>
>> Hi,
>>
>> I'm currently looking into the gspca_pac7302-driver and how it interacts
>> with libv4lconvert.
>> This is how it currently works
>> - driver announces v4l2_pix_format 640x480 (width x height)
>> - the frames (jpeg) passed to userspace are encoded as 480x640 and this
>> complies with the jpeg-header we generate
>> - libv4lconvert checks width/height in the jpeg-header and compares them
>> with the image format announced by the kernel:
>>     a) values are the same:
>>        1) V4LCONTROL_ROTATED_90_JPEG is NOT set for the device (standard
>> case):
>>            => everything is fine, image is decoded
>>        2) V4LCONTROL_ROTATED_90_JPEG is set for the device:
>>            => libv4lconvert bails out with -EIO displaying the error
>> message "unexpected width / height in JPEG header: expected: 640x480,
>> header: 480x640"
>>     b) values are different:
>>        1) V4LCONTROL_ROTATED_90_JPEG is NOT set:
>>            => libv4lconvert bails out with -EIO displaying the error
>> message "unexpected width / height in JPEG header: expected: 640x480,
>> header: 480x640"
>>        2) V4LCONTROL_ROTATED_90_JPEG is set:
>>            => image is decoded and rotated correctly
>>
>>
>> Thinking about this for some minutes:
>>
>> 1) shouldn't the kernel always announce the real image format (size) of
>> the data it passes to userspace ?
>
> It is passing the real size, the data is just in a vary funky format
> which
> needs rotation as part of its "decoding" / decompression.

It is first decoded, then rotated, right ?
Are the frames encoded as 480x640 (that's what the header says) or 640x480 ?
If the header is wrong, everything is fine. Otherwise we are passing the
size we finally get AFTER decoding and rotation to userspace, which I
think would be inconsistent.

>
>> Current behavior seems inconsistent to me...
>> Announcing the actual image size allows applications which trust the API
>> value more than the value in the frame header to decode the image
>> correctly without using libv4lconvert (although the image would still be
>> rotated).
>
> That assumes that the app would know how to decompress the data which it
> will not know, these cams do not generate standard JPEG data,
> libv4lconvert's
> decompression code is the only decompression code for this fsck-ed up
> JPEG-s,
> short of the windows drivers code.

Ok, that's true.
Because of the special format, applications are forced to use
libv4lconvert, so there is CURRENTLY no need to think about the kernel
<-> userspace interface.
But that might change in the future...

>
>> 2) shouldn't libv4lconvert always rotate the image if
>> V4LCONTROL_ROTATED_90_JPEG is set for a device ?
>> It seems like a2) is a bug, because the expected size should be 640x480,
>> too.
>
> rotating by 90 degrees also swaps the width and height, which are usually
> not the same, so rotating an image which starts at 640x480 will yield
> a final image of 480x640 which will not be what the app expects.

Well, I can't see why 480x640 should be an invalid format !? Depends on
the hardware.
Applications really shouldn't rely on width beeing always larger then
height. Otherwise I would call them buggy.

Regards,
Frank


>
>> 3) because all pac7302 devices are sending rotated image data, we should
>> add them ALL to libv4lconvert. Currently only 4 of the 14 devices are on
>> the list.
>> Do you want me to send a patch ?
>
> I see you've send a patch in the mean time, I'll reply in more detail to
> this to the patch mail.
>
> Regards,
>
> Hans

Re: pac7302-webcams and libv4lconvert interaction

[Hans de Goede]

Hi,

On 09/10/2012 05:36 PM, Frank Schäfer wrote:
> Hi Hans,
>
> Am 09.09.2012 23:20, schrieb Hans de Goede:
>> Hi,
>>
>> On 09/06/2012 05:13 PM, Frank Schäfer wrote:
>>>
>>> Hi,
>>>
>>> I'm currently looking into the gspca_pac7302-driver and how it interacts
>>> with libv4lconvert.
>>> This is how it currently works
>>> - driver announces v4l2_pix_format 640x480 (width x height)
>>> - the frames (jpeg) passed to userspace are encoded as 480x640 and this
>>> complies with the jpeg-header we generate
>>> - libv4lconvert checks width/height in the jpeg-header and compares them
>>> with the image format announced by the kernel:
>>>      a) values are the same:
>>>         1) V4LCONTROL_ROTATED_90_JPEG is NOT set for the device (standard
>>> case):
>>>             => everything is fine, image is decoded
>>>         2) V4LCONTROL_ROTATED_90_JPEG is set for the device:
>>>             => libv4lconvert bails out with -EIO displaying the error
>>> message "unexpected width / height in JPEG header: expected: 640x480,
>>> header: 480x640"
>>>      b) values are different:
>>>         1) V4LCONTROL_ROTATED_90_JPEG is NOT set:
>>>             => libv4lconvert bails out with -EIO displaying the error
>>> message "unexpected width / height in JPEG header: expected: 640x480,
>>> header: 480x640"
>>>         2) V4LCONTROL_ROTATED_90_JPEG is set:
>>>             => image is decoded and rotated correctly
>>>
>>>
>>> Thinking about this for some minutes:
>>>
>>> 1) shouldn't the kernel always announce the real image format (size) of
>>> the data it passes to userspace ?
>>
>> It is passing the real size, the data is just in a vary funky format
>> which
>> needs rotation as part of its "decoding" / decompression.
>
> It is first decoded, then rotated, right ?

Yes, because rotating raw JPEG data is hard, but we believe that the
rotation happening inside the cam is a side-effect (or rather a bug) of how
the hardware encoding works. So that we split the 2 steps in software is just
for convenience. The fact is that these cams have a sensor which has 640x480
pixels, not 480x640. So the correct size to report to userspace is 640x480,
the rotation is a side effect of the special pixart pixel format / encoding.

> Are the frames encoded as 480x640 (that's what the header says) or 640x480 ?
> If the header is wrong, everything is fine. Otherwise we are passing the
> size we finally get AFTER decoding and rotation to userspace, which I
> think would be inconsistent.

They are encoded as 480x640, which is why the header says that (otherwise
even more pixart hacks to the jpeg decoder in tinyjpeg.c would be necessary),
but again the sensor has a resolution of 640x480, not 480x640. So both the
reported resolution and the header are right, they just don't agree because
these (dirt cheap) cameras are really funky.

>
>>
>>> Current behavior seems inconsistent to me...
>>> Announcing the actual image size allows applications which trust the API
>>> value more than the value in the frame header to decode the image
>>> correctly without using libv4lconvert (although the image would still be
>>> rotated).
>>
>> That assumes that the app would know how to decompress the data which it
>> will not know, these cams do not generate standard JPEG data,
>> libv4lconvert's
>> decompression code is the only decompression code for this fsck-ed up
>> JPEG-s,
>> short of the windows drivers code.
>
> Ok, that's true.
> Because of the special format, applications are forced to use
> libv4lconvert, so there is CURRENTLY no need to think about the kernel
> <-> userspace interface.
> But that might change in the future...
>
>>
>>> 2) shouldn't libv4lconvert always rotate the image if
>>> V4LCONTROL_ROTATED_90_JPEG is set for a device ?
>>> It seems like a2) is a bug, because the expected size should be 640x480,
>>> too.
>>
>> rotating by 90 degrees also swaps the width and height, which are usually
>> not the same, so rotating an image which starts at 640x480 will yield
>> a final image of 480x640 which will not be what the app expects.
>
> Well, I can't see why 480x640 should be an invalid format !? Depends on
> the hardware.

What I'm trying to say is that for the image to be 640x480 after rotation
by 90 degrees, it has to be 480x640 before rotation. So since the raw data
needs rotation it reports itself as 480x640, where as the end result
reports itself as 640x480 ...


Regards,

Hans

Re: pac7302-webcams and libv4lconvert interaction

[Frank Schäfer]

Hi,

Am 10.09.2012 20:31, schrieb Hans de Goede:
> Hi,
>
> On 09/10/2012 05:36 PM, Frank Schäfer wrote:
>> Hi Hans,
>>
>> Am 09.09.2012 23:20, schrieb Hans de Goede:
>>> Hi,
>>>
>>> On 09/06/2012 05:13 PM, Frank Schäfer wrote:
>>>>
>>>> Hi,
>>>>
>>>> I'm currently looking into the gspca_pac7302-driver and how it
>>>> interacts
>>>> with libv4lconvert.
>>>> This is how it currently works
>>>> - driver announces v4l2_pix_format 640x480 (width x height)
>>>> - the frames (jpeg) passed to userspace are encoded as 480x640 and
>>>> this
>>>> complies with the jpeg-header we generate
>>>> - libv4lconvert checks width/height in the jpeg-header and compares
>>>> them
>>>> with the image format announced by the kernel:
>>>>      a) values are the same:
>>>>         1) V4LCONTROL_ROTATED_90_JPEG is NOT set for the device
>>>> (standard
>>>> case):
>>>>             => everything is fine, image is decoded
>>>>         2) V4LCONTROL_ROTATED_90_JPEG is set for the device:
>>>>             => libv4lconvert bails out with -EIO displaying the error
>>>> message "unexpected width / height in JPEG header: expected: 640x480,
>>>> header: 480x640"
>>>>      b) values are different:
>>>>         1) V4LCONTROL_ROTATED_90_JPEG is NOT set:
>>>>             => libv4lconvert bails out with -EIO displaying the error
>>>> message "unexpected width / height in JPEG header: expected: 640x480,
>>>> header: 480x640"
>>>>         2) V4LCONTROL_ROTATED_90_JPEG is set:
>>>>             => image is decoded and rotated correctly
>>>>
>>>>
>>>> Thinking about this for some minutes:
>>>>
>>>> 1) shouldn't the kernel always announce the real image format
>>>> (size) of
>>>> the data it passes to userspace ?
>>>
>>> It is passing the real size, the data is just in a vary funky format
>>> which
>>> needs rotation as part of its "decoding" / decompression.
>>
>> It is first decoded, then rotated, right ?
>
> Yes, because rotating raw JPEG data is hard, but we believe that the
> rotation happening inside the cam is a side-effect (or rather a bug)
> of how
> the hardware encoding works. So that we split the 2 steps in software
> is just
> for convenience. The fact is that these cams have a sensor which has
> 640x480
> pixels, not 480x640. So the correct size to report to userspace is
> 640x480,
> the rotation is a side effect of the special pixart pixel format /
> encoding.

I don't get it.
What's the definition of the width+height values in v4l2_pix_format ?
Does it really describe the pixel layout on the sensor ?
Up to now, I thought it describes the size of the frames passed to
userspace !
And if the images are encoded in 480x640, v4l2_pix_format should report
480x640, too !?

What are we going to do if we find a pac7302 device with the sensor
mounted rotated ?
Following the current approach, we would need to modify the kernel to
report 480x640 in this case. But the sensor is still 640x480, right ?

>
>> Are the frames encoded as 480x640 (that's what the header says) or
>> 640x480 ?
>> If the header is wrong, everything is fine. Otherwise we are passing the
>> size we finally get AFTER decoding and rotation to userspace, which I
>> think would be inconsistent.
>
> They are encoded as 480x640, which is why the header says that (otherwise
> even more pixart hacks to the jpeg decoder in tinyjpeg.c would be
> necessary),
> but again the sensor has a resolution of 640x480, not 480x640. So both
> the
> reported resolution and the header are right, they just don't agree
> because
> these (dirt cheap) cameras are really funky.

No, only the jpeg-header is right.
The kernel currently reports the image size we want to have in the end
(after rotating the image in userspace).
Do you really think this is correct API design ? Shouldn't both things
be kept separate ?

>
>>
>>>
>>>> Current behavior seems inconsistent to me...
>>>> Announcing the actual image size allows applications which trust
>>>> the API
>>>> value more than the value in the frame header to decode the image
>>>> correctly without using libv4lconvert (although the image would
>>>> still be
>>>> rotated).
>>>
>>> That assumes that the app would know how to decompress the data
>>> which it
>>> will not know, these cams do not generate standard JPEG data,
>>> libv4lconvert's
>>> decompression code is the only decompression code for this fsck-ed up
>>> JPEG-s,
>>> short of the windows drivers code.
>>
>> Ok, that's true.
>> Because of the special format, applications are forced to use
>> libv4lconvert, so there is CURRENTLY no need to think about the kernel
>> <-> userspace interface.
>> But that might change in the future...
>>
>>>
>>>> 2) shouldn't libv4lconvert always rotate the image if
>>>> V4LCONTROL_ROTATED_90_JPEG is set for a device ?
>>>> It seems like a2) is a bug, because the expected size should be
>>>> 640x480,
>>>> too.
>>>
>>> rotating by 90 degrees also swaps the width and height, which are
>>> usually
>>> not the same, so rotating an image which starts at 640x480 will yield
>>> a final image of 480x640 which will not be what the app expects.
>>
>> Well, I can't see why 480x640 should be an invalid format !? Depends on
>> the hardware.
>
> What I'm trying to say is that for the image to be 640x480 after rotation
> by 90 degrees, it has to be 480x640 before rotation.

Sure. But I would say in both, v4l2_pix_format AND the header.

> So since the raw data
> needs rotation it reports itself as 480x640, where as the end result
> reports itself as 640x480 ...

Yes, but getting 480x640 raw data from the kernel doesn't necessarily
mean that the image needs rotation !

libv4lconvert should be modifed to do the rotation regardless of what
comes out of the kernel whenever V4LCONTROL_ROTATED flag is set.
This way it becomes just a normal software control (like software h/v-flip).
At the moment, it can only handle (jpeg) data where the kernel and
header sizes are different.

Regards,
Frank

>
>
> Regards,
>
> Hans

Re: pac7302-webcams and libv4lconvert interaction

[Hans de Goede]

Hi,

On 09/10/2012 10:24 PM, Frank Schäfer wrote:

<snip>

>
> libv4lconvert should be modifed to do the rotation regardless of what
> comes out of the kernel whenever V4LCONTROL_ROTATED flag is set.
> This way it becomes just a normal software control (like software h/v-flip).
> At the moment, it can only handle (jpeg) data where the kernel and
> header sizes are different.

And that cannot be done, because what if the app enumerates frame sizes, sees
640x480 there, then the rotate 90 degrees option gets toggled on, and it
starts streaming and gets 480x640 frames all of a sudden, or what if the rotation
changes during streaming ?

Which is exavtly the reason why rotated-90 is being handled the way it is, which
is I must admit a bit hacky, but that is what it is, just a hack for pac7302
cameras.

Doing general rotation support is hard, if not impossible, at the v4l2 level since
it changes not only the contents but also the dimensions of the image.

Regards,

Hans

Re: pac7302-webcams and libv4lconvert interaction

[Frank Schäfer]

Am 11.09.2012 09:29, schrieb Hans de Goede:
> Hi,
>
> On 09/10/2012 10:24 PM, Frank Schäfer wrote:
>
> <snip>

Ok, I understand what that means...

>
>>
>> libv4lconvert should be modifed to do the rotation regardless of what
>> comes out of the kernel whenever V4LCONTROL_ROTATED flag is set.
>> This way it becomes just a normal software control (like software
>> h/v-flip).
>> At the moment, it can only handle (jpeg) data where the kernel and
>> header sizes are different.
>
> And that cannot be done, because what if the app enumerates frame
> sizes, sees
> 640x480 there, then the rotate 90 degrees option gets toggled on, and it
> starts streaming and gets 480x640 frames all of a sudden, or what if
> the rotation
> changes during streaming ?
>
> Which is exavtly the reason why rotated-90 is being handled the way it
> is, which
> is I must admit a bit hacky, but that is what it is, just a hack for
> pac7302
> cameras.
>
> Doing general rotation support is hard, if not impossible, at the v4l2
> level since
> it changes not only the contents but also the dimensions of the image.

Ok, you're right, general rotation / toggling rotation while streaming
is indeed a problem.
Anyway, I can't see how this affects the frame size reported by the
kernel. The data format coming out the kernel is always the same, it
doesn,'t matter if we rotate or not rotate the image (either statically
or dynamically) in userspace.

The only advantage with the current solution is, that we can pass the
frame size information from the kernel to userspace directly.
But with the cost of an inconsistent API.

And a negative side effect is, that unknown pac7302 devices (with no
V4LCONTROL_ROTATED_90_JPEG entry in libv4lconvert) do not work.
With a consistent API behavior, they would work fine (output a rotated
image). Users would at least know that their device is working and most
of them know what to do next.
For image rotation, we still need to add an entry to libv4lconvert and
to modify it to invert the width and height values in v4l2_pix_format in
this case.

The device I have here is a good example: many people reported this
device as not working years ago, one of them even got a hint in a forum
that this could be a pac7302 device 2 years ago.
But with the gpsca-pac7302 driver, he got no picture and gave up.
And if I had not started q4vl2 from the terminal and had noticed the
error message from libv4lconvert, I would have needed much more time to
find out what's wrong...

Regards,
Frank

>
> Regards,
>
> Hans

Re: pac7302-webcams and libv4lconvert interaction

[Hans de Goede]

Hi,

On 09/12/2012 04:36 PM, Frank Schäfer wrote:

<snip>

>
> And a negative side effect is, that unknown pac7302 devices (with no
> V4LCONTROL_ROTATED_90_JPEG entry in libv4lconvert) do not work.
> With a consistent API behavior, they would work fine (output a rotated
> image). Users would at least know that their device is working and most
> of them know what to do next.
> For image rotation, we still need to add an entry to libv4lconvert and
> to modify it to invert the width and height values in v4l2_pix_format in
> this case.

That is a good point, unfortunately we are stuck with how we are doing
things now, since changing things would break the kernel ABI.

Also ...

>
> The device I have here is a good example: many people reported this
> device as not working years ago, one of them even got a hint in a forum
> that this could be a pac7302 device 2 years ago.
> But with the gpsca-pac7302 driver, he got no picture and gave up.
> And if I had not started q4vl2 from the terminal and had noticed the
> error message from libv4lconvert, I would have needed much more time to
> find out what's wrong...

True, OTOH just having this fixed won't help a regular user, as he/she
would still need to first add the new usb-id to the pac7302 driver...

Regards,

Hans

Re: pac7302-webcams and libv4lconvert interaction

[Frank Schäfer]

Hi,

Am 13.09.2012 14:05, schrieb Hans de Goede:
> Hi,
>
> On 09/12/2012 04:36 PM, Frank Schäfer wrote:
>
> <snip>
>
>>
>> And a negative side effect is, that unknown pac7302 devices (with no
>> V4LCONTROL_ROTATED_90_JPEG entry in libv4lconvert) do not work.
>> With a consistent API behavior, they would work fine (output a rotated
>> image). Users would at least know that their device is working and most
>> of them know what to do next.
>> For image rotation, we still need to add an entry to libv4lconvert and
>> to modify it to invert the width and height values in v4l2_pix_format in
>> this case.
>
> That is a good point, unfortunately we are stuck with how we are doing
> things now, since changing things would break the kernel ABI.

Yeah, we can't break things. But I think this would be ABI fixing not
breaking. ;)
Actually...I'm not sure if this would be really an ABI change, because
the interface itself wouldn't change.
We would only fix a driver which passes a wrong value to userspace.
Its a question of interpretation...

>
> Also ...
>
>>
>> The device I have here is a good example: many people reported this
>> device as not working years ago, one of them even got a hint in a forum
>> that this could be a pac7302 device 2 years ago.
>> But with the gpsca-pac7302 driver, he got no picture and gave up.
>> And if I had not started q4vl2 from the terminal and had noticed the
>> error message from libv4lconvert, I would have needed much more time to
>> find out what's wrong...
>
> True, OTOH just having this fixed won't help a regular user, as he/she
> would still need to first add the new usb-id to the pac7302 driver...

Regular users, sure. But it would be a big step forward.
Adding new device IDs for testing is one of the easier things in the
Unix world.

Hans, I have a bunch of smaller things on my ToDo list which I want to
do first.
For now: maybe we can improve things by trusting the jpeg-header ?
That would mean removing the following section from
v4lconvert_decode_jpeg_tinyjpeg() :

    if (data->control_flags & V4LCONTROL_ROTATED_90_JPEG) {
        unsigned int tmp = width;
        width = height;
        height = tmp;
    }

    if (header_width != width || header_height != height) {
        V4LCONVERT_ERR("unexpected width / height in JPEG header: "
                   "expected: %ux%u, header: %ux%u\n",
                   width, height, header_width, header_height);
        errno = EIO;
        return -1;
    }


V4LCONTROL_ROTATED_90_JPEG is only used for the pac7302 devices and we
know that their header is correct.
And even in cases where the header is wrong, I think it would we better
to get a garbeled picture instead of -EIO.

Regards,
Frank

>
> Regards,
>
> Hans

Re: pac7302-webcams and libv4lconvert interaction

[Hans de Goede]

Hi,

On 09/16/2012 02:21 PM, Frank Schäfer wrote:
> Hi,
>
> Am 13.09.2012 14:05, schrieb Hans de Goede:
>> Hi,
>>
>> On 09/12/2012 04:36 PM, Frank Schäfer wrote:
>>
>> <snip>
>>
>>>
>>> And a negative side effect is, that unknown pac7302 devices (with no
>>> V4LCONTROL_ROTATED_90_JPEG entry in libv4lconvert) do not work.
>>> With a consistent API behavior, they would work fine (output a rotated
>>> image). Users would at least know that their device is working and most
>>> of them know what to do next.
>>> For image rotation, we still need to add an entry to libv4lconvert and
>>> to modify it to invert the width and height values in v4l2_pix_format in
>>> this case.
>>
>> That is a good point, unfortunately we are stuck with how we are doing
>> things now, since changing things would break the kernel ABI.
>
> Yeah, we can't break things. But I think this would be ABI fixing not
> breaking. ;)
> Actually...I'm not sure if this would be really an ABI change, because
> the interface itself wouldn't change.
> We would only fix a driver which passes a wrong value to userspace.
> Its a question of interpretation...

Well userspace will no longer work after the change, unless it gets updated
in sync, so its an ABI break clear and simple.

>
>>
>> Also ...
>>
>>>
>>> The device I have here is a good example: many people reported this
>>> device as not working years ago, one of them even got a hint in a forum
>>> that this could be a pac7302 device 2 years ago.
>>> But with the gpsca-pac7302 driver, he got no picture and gave up.
>>> And if I had not started q4vl2 from the terminal and had noticed the
>>> error message from libv4lconvert, I would have needed much more time to
>>> find out what's wrong...
>>
>> True, OTOH just having this fixed won't help a regular user, as he/she
>> would still need to first add the new usb-id to the pac7302 driver...
>
> Regular users, sure. But it would be a big step forward.
> Adding new device IDs for testing is one of the easier things in the
> Unix world.
>
> Hans, I have a bunch of smaller things on my ToDo list which I want to
> do first.
> For now: maybe we can improve things by trusting the jpeg-header ?
> That would mean removing the following section from
> v4lconvert_decode_jpeg_tinyjpeg() :
>
>      if (data->control_flags & V4LCONTROL_ROTATED_90_JPEG) {
>          unsigned int tmp = width;
>          width = height;
>          height = tmp;
>      }
>
>      if (header_width != width || header_height != height) {
>          V4LCONVERT_ERR("unexpected width / height in JPEG header: "
>                     "expected: %ux%u, header: %ux%u\n",
>                     width, height, header_width, header_height);
>          errno = EIO;
>          return -1;
>      }
>
>
> V4LCONTROL_ROTATED_90_JPEG is only used for the pac7302 devices and we
> know that their header is correct.
> And even in cases where the header is wrong, I think it would we better
> to get a garbeled picture instead of -EIO.

We're not just getting EIO, we're also logging the error to stderr, and
getting that error message in a bug report is a lot more useful then
getting a bug report about a "garbled picture" the end result for the
user is the same: "his / her cam is not usable", and either they file
a bug report or they don't.

Also a garbled picture is the *best* outcome, chances are that if things
don't match they get a crash instead of the error to stderr, which is
much much harder to debug.

Regards,

Hans