qv4l2-bug / libv4lconvert API issue

Hi,

I've noticed the following issues/bugs while playing with qv4l:
1.) with pac7302-webcams, only the RGB3 (RGB24) format is working. BGR3, YU12 and YV12 are broken
2.) for upside-down-mounted devices with an entry in libv4lconvert, automatic h/v-flipping doesn't work with some formats

I've been digging a bit deeper into the code and it seems that both issues are caused by a problem with the libv4lconvert-API:
Besides image format conversion, function v4lconvert_convert() also does the automatic image flipping and rotation (for devices with flags V4LCONTROL_HFLIPPED, V4LCONTROL_VFLIPPED and V4LCONTROL_ROTATED_90_JPEG) The problem is, that this function can be called multiple times for the same frame, which then of course results in repeated flipping and rotation...

And this is exactly what happens with qv4l2:
qv4l2 gets the frame from libv4l2, which calls v4lconvert_convert() in v4l2_dequeue_and_convert() or v4l2_read_and_convert().
The retrieved frame has the requested format and is already flipped/rotated. qv4l2 then calls v4lconvert_convert() again directly to convert the frame to RGB24 for GUI-output and this is where things are going wrong. In case of h/v-flip, the double conversion "only" equalizes the V4LCONTROL_HFLIPPED, V4LCONTROL_VFLIPPED flags, but for rotated devices, the image gets corrupted.

Sure, what qv4l2 does is a crazy. Applications usually request the format needed for GUI-output directly from libv4l2. Anyway, as long as it is valid to call libv4lconvert directly, we can not assume that v4lconvert_convert() is called only one time.

At the moment, I see no possibility to fix this without changing the libv4lconvert-API.
Thoughts ?

Hi,

Quoted 33 lines · Frank Schäfer

Hi,

I've noticed the following issues/bugs while playing with qv4l:
1.) with pac7302-webcams, only the RGB3 (RGB24) format is working. BGR3, YU12 and YV12 are broken
2.) for upside-down-mounted devices with an entry in libv4lconvert, automatic h/v-flipping doesn't work with some formats

I've been digging a bit deeper into the code and it seems that both issues are caused by a problem with the libv4lconvert-API:
Besides image format conversion, function v4lconvert_convert() also does the automatic image flipping and rotation (for devices with flags V4LCONTROL_HFLIPPED, V4LCONTROL_VFLIPPED and V4LCONTROL_ROTATED_90_JPEG) The problem is, that this function can be called multiple times for the same frame, which then of course results in repeated flipping and rotation...

And this is exactly what happens with qv4l2:
qv4l2 gets the frame from libv4l2, which calls v4lconvert_convert() in v4l2_dequeue_and_convert() or v4l2_read_and_convert().
The retrieved frame has the requested format and is already flipped/rotated.
qv4l2 then calls v4lconvert_convert() again directly to convert the frame to RGB24 for GUI-output and this is where things are going wrong. In case of h/v-flip, the double conversion "only" equalizes the V4LCONTROL_HFLIPPED, V4LCONTROL_VFLIPPED flags, but for rotated devices, the image gets corrupted.

Sure, what qv4l2 does is a crazy. Applications usually request the format needed for GUI-output directly from libv4l2. Anyway, as long as it is valid to call libv4lconvert directly, we can not assume that v4lconvert_convert() is called only one time.

Quoted 3 lines

At the moment, I see no possibility to fix this without changing the libv4lconvert-API.
Thoughts ?

What you've found is a qv4l2 bug (do you have the latest version?)

one
is supposed to either use libv4l2, or do raw device access and then call libv4lconvert directly.

These are also the 2 modes qv4l2 has
(for testing purposes), it is not supposed to do the manual convert call when using libv4l2 to access the device ...

Regards,

Hans

Quoted 1 line

What you've found is a qv4l2 bug (do you have the latest version?)

Of course, I'm using the latest developer version.

Even if this is just a qv4l2-bug: how do you want to fix it without removing the format selction feature ?

Quoted 4 lines

one
is supposed to either use libv4l2, or do raw device access and then call libv4lconvert directly.

Even when using libv4lconvert only, multiple frame conversions are still causing the same trouble.

Hans, first of all, I would like to say that my intention is not to savage your work.
I know API design and maintanance is very difficult and you are doing a great job.
Things like this just happen and we have to make the best out of it.

But saying that libv4l2 and libv4lconvert can't be used at the same (although they are separate public libraries) and that v4lconvert_convert() may only be called once per frame seems to me like a - I would say "weird" - reinterpretation of the API...
I don't think this is what applications currently expect (qv4l2 doesn't ;) ) and at least this would need public clarification.

So let's see if there is a possibility to fix the issue by improving the libraries without breaking the API.

What about the following solution:
- make v4lconvert_convert_pixfmt() and v4lconvert_crop() public functions and fix qv4l2 to use them instead of v4lconvert_convert()
- also make the flip and rotation functions (v4lconvert_flip(), v4lconvert_rotate90()) publicly available

- modify libv4l2 to call v4lconvert_convert_pixfmt() and the flip+rotation+crop functions instead of v4lconvert_convert()

- fix v4lconvert_convert() to not do transparent image flipping/rotation (means => call v4lconvert_convert_pixfmt() and v4lconvert_crop() only).

For API-clean-up:
- create a copy of (the fixed) v4lconvert_convert() called something like v4lconvert_convert_crop()
- declare v4lconvert_convert() as deprecated so that we can remove it sometime in the future

What do you think ?

Hi,

On 09/27/2012 03:20 PM, Frank Schäfer wrote:

<snip>

Quoted 2 lines

Quoted 1 line

What you've found is a qv4l2 bug (do you have the latest version?)

Quoted 4 lines

Of course, I'm using the latest developer version.

Even if this is just a qv4l2-bug: how do you want to fix it without removing the format selction feature ?

Well, if qv4l2 can only handle rgb24 data, then it should gray out the format selection (fixing it at rgb24) when not in raw mode.

v4lconvert_convert should only be called with
src_fmt and dest_fmt parameters which are the result of a v4lconvert_try_format call, which clearly is not the case here!

Quoted 8 lines

Quoted 5 lines

one
is supposed to either use libv4l2, or do raw device access and then call libv4lconvert directly.

Even when using libv4lconvert only, multiple frame conversions are still causing the same trouble.

True, but doing multiple conversions on one frame is just crazy ...

Quoted 6 lines

Hans, first of all, I would like to say that my intention is not to savage your work.
I know API design and maintanance is very difficult and you are doing a great job.
Things like this just happen and we have to make the best out of it.

I will gladly admit that since libv4lconvert has organically grown things like flipping and software processing, the API is no longer ideal :)

Quoted 8 lines

But saying that libv4l2 and libv4lconvert can't be used at the same (although they are separate public libraries) and that v4lconvert_convert() may only be called once per frame seems to me like a - I would say "weird" - reinterpretation of the API...
I don't think this is what applications currently expect (qv4l2 doesn't ;) ) and at least this would need public clarification.

Using the 2 at the same time never was the intention libv4lconvert lies *beneath* libv4l2 in the stack.

Using them both at the same time
would be like using some high level file io API, while at the same making lowlevel seek / read / write calls on the underlying fd and then expecting things to behave consistently. 00.9% of all apps should (and do) use the "highlevel" libv4l2 API. Some testing / developer apps like qv4l2 use libv4lconvert directly.

And I must admit that
I've considered simple making the libv4lconvert API private at times.

Quoted 10 lines

So let's see if there is a possibility to fix the issue by improving the libraries without breaking the API.

What about the following solution:
- make v4lconvert_convert_pixfmt() and v4lconvert_crop() public functions and fix qv4l2 to use them instead of v4lconvert_convert()
- also make the flip and rotation functions (v4lconvert_flip(), v4lconvert_rotate90()) publicly available

That would need some careful review of their API's but after that yes that should be doable.

Quoted 5 lines

- modify libv4l2 to call v4lconvert_convert_pixfmt() and the flip+rotation+crop functions instead of v4lconvert_convert()

- fix v4lconvert_convert() to not do transparent image flipping/rotation (means => call v4lconvert_convert_pixfmt() and v4lconvert_crop() only).

Erm, no, have you looked at v4lconvert_convert it does a lot of magic with figuring out how much intermediate buffers it needs (skipping steps where possible), caches those buffers rather then re-alloc
them every frame, etc.

Making it do less means loosing some chances for optimization, and frankly
it works well. I don't see why we would need some do some stuff but not all
function when we also offer all the separate steps for users who want them.

Also I still consider the rotate 90 for pac7302 part of the pixfmt conversion,
this is something specific to how these cameras encode the picture, not a generic thing.

Quoted 7 lines

For API-clean-up:
- create a copy of (the fixed) v4lconvert_convert() called something like v4lconvert_convert_crop()
- declare v4lconvert_convert() as deprecated so that we can remove it sometime in the future

Quoted 1 line

What do you think ?

2 things:

1) qv4l2 needs to be fixed to not show to format selection in wrapped mode

2) What is the use case for having separate convert_pixfmt etc. functions available ... ?

Regards,

Hans

Hi,

Am 27.09.2012 21:41, schrieb Hans de Goede:

Quoted 10 lines

Hi,

On 09/27/2012 03:20 PM, Frank Schäfer wrote:

<snip>

Quoted 4 lines

Quoted 1 line

What you've found is a qv4l2 bug (do you have the latest version?)

Of course, I'm using the latest developer version.

Quoted 5 lines

Quoted 2 lines

Even if this is just a qv4l2-bug: how do you want to fix it without removing the format selction feature ?

Well, if qv4l2 can only handle rgb24 data, then it should gray out the format selection (fixing it at rgb24) when not in raw mode.

So you say "just remove this feature from qv4l2".
I prefer fixing the library / API instead.

Quoted 5 lines

v4lconvert_convert should only be called with
src_fmt and dest_fmt parameters which are the result of a v4lconvert_try_format call, which clearly is not the case here!

Why ? Because our library code is broken ?

Is this important restriction documented somewhere ?

Quoted 11 lines

Quoted 10 lines

Quoted 7 lines

one
is supposed to either use libv4l2, or do raw device access and then call libv4lconvert directly.

Even when using libv4lconvert only, multiple frame conversions are still causing the same trouble.

Quoted 1 line

True, but doing multiple conversions on one frame is just crazy ...

Well, I would say "not necessary in most cases". But I can certainly think of use cases (other than what qv4l2 does).
At least it should be possible and I think this is what application programmers expect when using a conversion function from a library.

Quoted 9 lines

Quoted 6 lines

But saying that libv4l2 and libv4lconvert can't be used at the same (although they are separate public libraries) and that v4lconvert_convert() may only be called once per frame seems to me like a - I would say "weird" - reinterpretation of the API...
I don't think this is what applications currently expect (qv4l2 doesn't ;) ) and at least this would need public clarification.

Using the 2 at the same time never was the intention libv4lconvert lies *beneath* libv4l2 in the stack.

Yeah, sure.

Quoted 6 lines

Using them both at the same time
would be like using some high level file io API, while at the same making lowlevel seek / read / write calls on the underlying fd and then expecting things to behave consistently. 00.9% of all apps should (and do) use the "highlevel" libv4l2 API. Some testing / developer apps like qv4l2 use libv4lconvert directly.

The problem here is, that we actually have TWO high level APIs which interact in a - let's call it "unfortunate" - way.

This interaction is not clear for the users (due to the transparent processing stuff) and it doesn't match not what users expect. But I think we can fix it and gain some nice extra features as a bonus.

Quoted 2 lines

2) What is the use case for having separate convert_pixfmt etc. functions available ... ?

We already have them as separate functions, so the only question is:
does it make sense to make them public ?
I would say: yes, because this seems to be a sane way to fix a problem. And the second reason would be, that the provided operations are usefull for applications.

Hi,

Quoted 19 lines · Frank Schäfer

Hi,

Am 27.09.2012 21:41, schrieb Hans de Goede:

Quoted 15 lines

Hi,

On 09/27/2012 03:20 PM, Frank Schäfer wrote:

<snip>

Quoted 6 lines

Quoted 1 line

What you've found is a qv4l2 bug (do you have the latest version?)

Of course, I'm using the latest developer version.

Even if this is just a qv4l2-bug: how do you want to fix it without removing the format selction feature ?

Well, if qv4l2 can only handle rgb24 data, then it should gray out the format selection (fixing it at rgb24) when not in raw mode.

Quoted 2 lines

So you say "just remove this feature from qv4l2".
I prefer fixing the library / API instead.

No I'm suggesting to keep the feature to select which input format to use when in raw mode, while at the same time disabling the feature) when in libv4l2 mode. What use is it to ask libv4l2 for say YUV420 data and then later ask libv4lconvert to convert this to RGB24, when you could have asked libv4l2 for RGB24 right away.

...

Quoted 21 lines

Quoted 11 lines

Quoted 8 lines

But saying that libv4l2 and libv4lconvert can't be used at the same (although they are separate public libraries) and that v4lconvert_convert() may only be called once per frame seems to me like
a - I would say "weird" - reinterpretation of the API... I don't think this is what applications currently expect (qv4l2 doesn't
;) ) and at least this would need public clarification.

Using the 2 at the same time never was the intention libv4lconvert lies *beneath* libv4l2 in the stack.

Yeah, sure.

Quoted 6 lines

Using them both at the same time
would be like using some high level file io API, while at the same making lowlevel seek / read / write calls on the underlying fd and then expecting things to behave consistently. 00.9% of all apps should (and do) use the "highlevel" libv4l2 API. Some testing / developer apps like qv4l2 use libv4lconvert directly.

Quoted 2 lines

The problem here is, that we actually have TWO high level APIs which interact in a - let's call it "unfortunate" - way.

I disagree that they are both highlevel. I know of only 2 tools which use libv4lconvert directly, qv4l2 and svv, and both of them were written by
kernel developers for driver testing. So in practice everyone who want a "high" level API is using libv4l2 (which is already low-level enough by it self).

Quoted 4 lines

This interaction is not clear for the users (due to the transparent processing stuff) and it doesn't match not what users expect. But I think we can fix it and gain some nice extra features as a bonus.

Then lets document the interaction. I think you don't realize which can of worms you're opening when you try to make libv4lconvert more generally usable.

Please read the v4lconvert_convert function very carefully, esp. the part where it hooks into v4lprocessing (software whitebalance, and gamma correction). Make sure you understand what v4lprocessing_pre_processing() does, what the difference is between convert = 1 and convert = 2, and where *all* the callers are of v4lprocessing_processing() (hint one is hiding in v4lconvert_convert_pixfmt)

Make sure you understand every single line of v4lconvert_convert! Once you've done that I will welcome a proposal to make the libv4lconvert API more general usable which:

1) Does not break any existing use-cases
2) Does not slow down things in anyway (so which does not introduce any extra intermediate buffers)

<snip>

Quoted 8 lines

Quoted 2 lines

2) What is the use case for having separate convert_pixfmt etc. functions available ... ?

We already have them as separate functions, so the only question is:
does it make sense to make them public ?
I would say: yes, because this seems to be a sane way to fix a problem. And the second reason would be, that the provided operations are usefull for applications.

My point is that currently some of them have side-effects, ie v4lconvert_convert_pixfmt() calling v4lprocessing_processing(), which is fine as is, but may be a problem when making it public.

I agree that the libv4lconvert API could use an overhaul, but one of the first things to do would be to untangle the rather glued on atm libv4lcontrol and libv4lprocessing bits. I have a (partial) plan for this,
but no time. One of the first things which I would like to do is to move over the software processing bits to a proper plugin model, where by the plugins themselves also add the necessary "fake" v4l2-ctrls for their own piece of functionality and where by the giant quirk list in libv4lconvert/control/libv4lcontrol.c becomes split in a quirk list per processing plugin, enabling that plugin for the cameras which need that plugin. libv4lcontrol.c would then just offer a generic mechanism to add fake controls, and to check if a camera matches *a* quirklist, which the plugins can then use.

This would then allow for much easier addition of new software processing plugins, such as for example software autofocus which is needed for some cams.

All of this can be done without any external API changes atm, and would have real tangible results, esp. once we start adding more software processing plugins. Where as exporting more of the libv4lcontrol internals,
will make it much harder to do this much needed overhaul, and buys us very little tangible advantages (again no normal end user applications are using libv4lconvert directly).

So what I would like to see happen, if you want to work on libv4lconvert is
to start with doing the much needed overhaul of the internals, and once that
is done making a more generic API available is fine, and if we break the API
in the process that is fine too. But lets start with doing the ground work
on not with exporting internals which are not suitable for exporting atm.

Note that if there were a real need for exporting these internals right now,
things would be different, but I don't see such a real need, and no, sorry,
from my pov qv4l2 does not count, esp. since things can easily be fixed there.

Note that I've written a proposal for a libv4lprocessing plugins a long time
ago, you can find it here:
http://www.mail-archive.com/[email protected]/msg18993.html

Regards,

Hans

Hi Hans,

Am 30.09.2012 11:54, schrieb Hans de Goede:

Quoted 25 lines

Hi,

Quoted 21 lines · Frank Schäfer

Hi,

Am 27.09.2012 21:41, schrieb Hans de Goede:

Quoted 8 lines

Hi,

On 09/27/2012 03:20 PM, Frank Schäfer wrote:

<snip>

Quoted 2 lines

Quoted 1 line

What you've found is a qv4l2 bug (do you have the latest version?)

Quoted 7 lines

Quoted 4 lines

Of course, I'm using the latest developer version.

Even if this is just a qv4l2-bug: how do you want to fix it without removing the format selction feature ?

Well, if qv4l2 can only handle rgb24 data, then it should gray out the format selection (fixing it at rgb24) when not in raw mode.

So you say "just remove this feature from qv4l2".
I prefer fixing the library / API instead.

Quoted 5 lines

No I'm suggesting to keep the feature to select which input format to use when in raw mode, while at the same time disabling the feature) when in libv4l2 mode. What use is it to ask libv4l2 for say YUV420 data and then later ask libv4lconvert to convert this to RGB24, when you could have asked libv4l2 for RGB24 right away.

I assume the idea behind input format selction when using libv4l2 is to provide a possibilty to test libv4l2 ?

Quoted 29 lines

Hi Hans,

Am 30.09.2012 11:54, schrieb Hans de Goede:

Quoted 26 lines

Hi,

Quoted 10 lines · Frank Schäfer

Hi,

Am 27.09.2012 21:41, schrieb Hans de Goede:

Quoted 7 lines

Hi,

On 09/27/2012 03:20 PM, Frank Schäfer wrote:

<snip>

Quoted 1 line

Quoted 1 line

What you've found is a qv4l2 bug (do you have the latest version?)

Quoted 8 lines

Quoted 6 lines

Quoted 4 lines

Of course, I'm using the latest developer version.

Even if this is just a qv4l2-bug: how do you want to fix it without removing the format selction feature ?

Well, if qv4l2 can only handle rgb24 data, then it should gray out the format selection (fixing it at rgb24) when not in raw mode.

So you say "just remove this feature from qv4l2".
I prefer fixing the library / API instead.

No I'm suggesting to keep the feature to select which input format to use when in raw mode, while at the same time disabling the feature) when in libv4l2 mode. What use is it to ask libv4l2 for say YUV420 data and then later ask libv4lconvert to convert this to RGB24, when you could have asked libv4l2 for RGB24 right away.

Quoted 2 lines

I assume the idea behind input format selction when using libv4l2 is to provide a possibilty to test libv4l2 ?

The main reason why I show all formats is that the driver reports all these formats, so one should be able to select them in order to test the driver.

And I'm using libv4l2convert so that I can actually see a picture. For formats like MPEG that are unsupported by libv4l2convert I just dump the 'image' as is.

It is counterintuitive if a YUV format is converted to a proper picture using qv4l2 -r, but that it is all wrong with qv4l2.

I'm all for improving the library.

Regards,

Hans