qv4l2-bug / libv4lconvert API issue

qv4l2-bug / libv4lconvert API issue

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

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

Regards,
Frank

Re: qv4l2-bug / libv4lconvert API issue

[Hans de Goede]

Hi,

On 09/26/2012 11:04 PM, Frank Schäfer wrote:
> 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 ?

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

Re: qv4l2-bug / libv4lconvert API issue

[Frank Schäfer]

Hi,

Am 27.09.2012 12:26, schrieb Hans de Goede:
> Hi,
>
> On 09/26/2012 11:04 PM, Frank Schäfer wrote:
>> 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 ?
>
> 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 ?

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

Regards,
Frank



> 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

Re: qv4l2-bug / libv4lconvert API issue

[Hans de Goede]

Hi,

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

<snip>

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

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!

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

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

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

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

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

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

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

Re: qv4l2-bug / libv4lconvert API issue

[Frank Schäfer]

Hi,

Am 27.09.2012 21:41, schrieb Hans de Goede:
> Hi,
>
> On 09/27/2012 03:20 PM, Frank Schäfer wrote:
>
> <snip>
>
>>> 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.

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

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


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

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.

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

So let's improve 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.
>
> Using the 2 at the same time never was the intention libv4lconvert
> lies *beneath* libv4l2 in the stack. 

Yeah, sure.

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

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

:D
That would certainly make things consistent ;)

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

Did you notice the mail I've sent a few minutes later ? ;)
I agree, we have to keep it as is but should mark it as deprecated.

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

Yes, but why not make it a generic feature ? Would be nice to have.
(ever had a webcam with a clamp socket ?)
But this is just about a side effect, lets concentrate on the main issue.

>
>> 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 ?
>
> 2 things:
>
> 1) qv4l2 needs to be fixed to not show to format selection in wrapped
> mode

Or fix the library API behind.

>
> 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.
Conrete (as discussed):
we
- can keep qv4l2 as is (making a broken feature work instead of removing
it) ;)
- avoid bugs in other appliactions
- allow applications to easily implement for example image rotation
- ...

I'm certainly also aware of the basic API design rule "keep it clear and
simple and don't bloat it with unneeded stuff". Otherwise you can easily
end up with a maintainance nightmare.


Hans, I will be busy this weekend and probably the first half of the
next week, but I will come back to this.
Have a nice weekend !

Regards,
Frank

>
> Regards,
>
> Hans

Re: qv4l2-bug / libv4lconvert API issue

[Hans de Goede]

Hi,

On 09/28/2012 07:09 PM, Frank Schäfer wrote:
> Hi,
>
> Am 27.09.2012 21:41, schrieb Hans de Goede:
>> Hi,
>>
>> On 09/27/2012 03:20 PM, Frank Schäfer wrote:
>>
>> <snip>
>>
>>>> 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.
>
> 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.

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

Because that is a pre-condition which v4lconvert_convert expects
to be met. Not meeting that pre-condition means operating outside
of the designated operating parameters, and as such weird things
may happen.

> Is this important restriction documented somewhere ?

Not explicitly, patches welcome.

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

Right, as said before libv4lconvert is not meant as a generic
format conversion library, and using it as such is not necessarily a
good idea as there are much better alternatives out there for doing
generic format conversion (both more flexible and faster).

More over libv4lconvert is specifically designed to be called once
per frame. Yes another restriction that needs documenting.

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

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

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

>> 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/linux-media@vger.kernel.org/msg18993.html

Regards,

Hans

Re: qv4l2-bug / libv4lconvert API issue

[Frank Schäfer]

Hi Hans,

Am 30.09.2012 11:54, schrieb Hans de Goede:
> Hi,
>
> On 09/28/2012 07:09 PM, Frank Schäfer wrote:
>> Hi,
>>
>> Am 27.09.2012 21:41, schrieb Hans de Goede:
>>> Hi,
>>>
>>> On 09/27/2012 03:20 PM, Frank Schäfer wrote:
>>>
>>> <snip>
>>>
>>>>> 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.
>>
>> 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.

I assume the idea behind input format selction when using libv4l2 is to
provide a possibilty to test libv4l2 ?
I'm not sure if we really need this.
If not, sure, we can simply remove it to get rid of the problem.



<snip> [we both agree that the current design/behavior is "suboptimal",
needs to be documented and should be improved]

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

Hmmm... that's a weird argumentation.
Because only a few applications are using it, it is not a high level API ?
And assuming that we really know all users of a public API is very
dangerous...


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

Yeah, it's a bit tricky, but understandable.
At the moment, the plan is not to change WHAT is done in this function,
but WHERE it is done.


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

Sure. Will do that.
Basically, the idea is to make libv4lconvert a pure toolbox with methods for
- for V4L_PIX_FORMAT conversion
- cropping
- horizontal+vertical flipping
- rotation
- processing (applying filters => auto whitebalance, auto gain, gamma
correction etc.)
- ...
of frames. Most of these methods are already there and only need to be
made public (some with small extenstions/modifiactions).

The whole "magic" (transparent flipping, rotation, ...) should be done
inside libv4l.
Does that make sense for you ?


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

Yes, I have to think about that.

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

Btw, did you ever think about making the device list a text file ?
You know editing a text file is much easer than updating the whole
library. ;)

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

I agree. First overhaul the internals, then extend the API.

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

100% agreement.

>
> 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/linux-media@vger.kernel.org/msg18993.html

Thanks, I will consider it.

Regards,
Frank

>
> Regards,
>
> Hans
>
>

Re: qv4l2-bug / libv4lconvert API issue

[Hans Verkuil]

On Wed 3 October 2012 12:22:48 Frank Schäfer wrote:
> Hi Hans,
> 
> Am 30.09.2012 11:54, schrieb Hans de Goede:
> > Hi,
> >
> > On 09/28/2012 07:09 PM, Frank Schäfer wrote:
> >> Hi,
> >>
> >> Am 27.09.2012 21:41, schrieb Hans de Goede:
> >>> Hi,
> >>>
> >>> On 09/27/2012 03:20 PM, Frank Schäfer wrote:
> >>>
> >>> <snip>
> >>>
> >>>>> 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.
> >>
> >> 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.
> 
> 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

Re: qv4l2-bug / libv4lconvert API issue

[Frank Schäfer]

Am 03.10.2012 15:32, schrieb Hans Verkuil:
> On Wed 3 October 2012 12:22:48 Frank Schäfer wrote:
>> Hi Hans,
>>
>> Am 30.09.2012 11:54, schrieb Hans de Goede:
>>> Hi,
>>>
>>> On 09/28/2012 07:09 PM, Frank Schäfer wrote:
>>>> Hi,
>>>>
>>>> Am 27.09.2012 21:41, schrieb Hans de Goede:
>>>>> Hi,
>>>>>
>>>>> On 09/27/2012 03:20 PM, Frank Schäfer wrote:
>>>>>
>>>>> <snip>
>>>>>
>>>>>>> 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.
>>>> 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.
>> 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.

Yes, but for pure testing of the driver output formats, it is better to
open the device in raw mode and convert the picture for GUI-output with
a v4lconvert_convert() call.

> 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 not sure I understand what you mean...

Regards,
Frank

> I'm all for improving the library.
>
> Regards,
>
> 	Hans