nasty bug at qv4l2

nasty bug at qv4l2

[Mauro Carvalho Chehab]

Hans V/Hans G,

There's a nasty bug at qv4l2 or at libv4l: it is not properly updating 
all info, if you change the video device. On my tests with uvcvideo (video0) 
and a gspca camera (pac7302, video1), it was showing the supported formats
for the uvcvideo camera when I changed from video0 to video1.

The net result is that the image were handled with the wrong decoder
(instead of using fourcc V4L2_PIX_FMT_PJPG, it were using BGR3), producing
a wrong decoding.

Could you please take a look on it?

Cheers,
Mauro

Re: nasty bug at qv4l2

[Hans de Goede]

Hi,

On 12/22/2010 12:30 PM, Mauro Carvalho Chehab wrote:
> Hans V/Hans G,
>
> There's a nasty bug at qv4l2 or at libv4l: it is not properly updating
> all info, if you change the video device. On my tests with uvcvideo (video0)
> and a gspca camera (pac7302, video1), it was showing the supported formats
> for the uvcvideo camera when I changed from video0 to video1.
>
> The net result is that the image were handled with the wrong decoder
> (instead of using fourcc V4L2_PIX_FMT_PJPG, it were using BGR3), producing
> a wrong decoding.
>
> Could you please take a look on it?

I'm pretty sure this is not a libv4l issue (other apps which allows witching
the source work fine), but rather a qv4l2 problem, esp. as it uses libv4lconvert
directly rather then going through libv4l (iirc).

Hans V, can you take a look at this?

Regards,

Hans

Re: nasty bug at qv4l2

[Hans Verkuil]

On Friday, December 24, 2010 15:19:26 Hans de Goede wrote:
> Hi,
> 
> On 12/22/2010 12:30 PM, Mauro Carvalho Chehab wrote:
> > Hans V/Hans G,
> >
> > There's a nasty bug at qv4l2 or at libv4l: it is not properly updating
> > all info, if you change the video device. On my tests with uvcvideo (video0)
> > and a gspca camera (pac7302, video1), it was showing the supported formats
> > for the uvcvideo camera when I changed from video0 to video1.
> >
> > The net result is that the image were handled with the wrong decoder
> > (instead of using fourcc V4L2_PIX_FMT_PJPG, it were using BGR3), producing
> > a wrong decoding.
> >
> > Could you please take a look on it?
> 
> I'm pretty sure this is not a libv4l issue (other apps which allows witching
> the source work fine), but rather a qv4l2 problem, esp. as it uses libv4lconvert
> directly rather then going through libv4l (iirc).

And I'm pretty sure it isn't a qv4l2 issue :-)

For the record: qv4l2 can open a device node either in 'raw' mode bypassing libv4l
and using v4lconvert to convert unsupported pixformats, or in 'wrapped' mode where
libv4l is used for all device node accesses.

> Hans V, can you take a look at this?

Not until I am back to Oslo at the beginning of January as all my webcams are there.

Regards,

	Hans

-- 
Hans Verkuil - video4linux developer - sponsored by Cisco

Re: nasty bug at qv4l2

[Hans de Goede]

Hi,

On 12/24/2010 03:20 PM, Hans Verkuil wrote:
> On Friday, December 24, 2010 15:19:26 Hans de Goede wrote:
>> Hi,
>>
>> On 12/22/2010 12:30 PM, Mauro Carvalho Chehab wrote:
>>> Hans V/Hans G,
>>>
>>> There's a nasty bug at qv4l2 or at libv4l: it is not properly updating
>>> all info, if you change the video device. On my tests with uvcvideo (video0)
>>> and a gspca camera (pac7302, video1), it was showing the supported formats
>>> for the uvcvideo camera when I changed from video0 to video1.
>>>
>>> The net result is that the image were handled with the wrong decoder
>>> (instead of using fourcc V4L2_PIX_FMT_PJPG, it were using BGR3), producing
>>> a wrong decoding.
>>>
>>> Could you please take a look on it?
>>
>> I'm pretty sure this is not a libv4l issue (other apps which allows witching
>> the source work fine), but rather a qv4l2 problem, esp. as it uses libv4lconvert
>> directly rather then going through libv4l (iirc).
>
> And I'm pretty sure it isn't a qv4l2 issue :-)
>
> For the record: qv4l2 can open a device node either in 'raw' mode bypassing libv4l
> and using v4lconvert to convert unsupported pixformats, or in 'wrapped' mode where
> libv4l is used for all device node accesses.
>

Interesting, how does it switch between the modes? Mauro were you using wrapped mode
or raw mode when you saw this ?

Regards,

hans

Re: nasty bug at qv4l2

[Hans Verkuil]

On Friday, December 24, 2010 15:41:10 Hans de Goede wrote:
> Hi,
> 
> On 12/24/2010 03:20 PM, Hans Verkuil wrote:
> > On Friday, December 24, 2010 15:19:26 Hans de Goede wrote:
> >> Hi,
> >>
> >> On 12/22/2010 12:30 PM, Mauro Carvalho Chehab wrote:
> >>> Hans V/Hans G,
> >>>
> >>> There's a nasty bug at qv4l2 or at libv4l: it is not properly updating
> >>> all info, if you change the video device. On my tests with uvcvideo (video0)
> >>> and a gspca camera (pac7302, video1), it was showing the supported formats
> >>> for the uvcvideo camera when I changed from video0 to video1.
> >>>
> >>> The net result is that the image were handled with the wrong decoder
> >>> (instead of using fourcc V4L2_PIX_FMT_PJPG, it were using BGR3), producing
> >>> a wrong decoding.
> >>>
> >>> Could you please take a look on it?
> >>
> >> I'm pretty sure this is not a libv4l issue (other apps which allows witching
> >> the source work fine), but rather a qv4l2 problem, esp. as it uses libv4lconvert
> >> directly rather then going through libv4l (iirc).
> >
> > And I'm pretty sure it isn't a qv4l2 issue :-)
> >
> > For the record: qv4l2 can open a device node either in 'raw' mode bypassing libv4l
> > and using v4lconvert to convert unsupported pixformats, or in 'wrapped' mode where
> > libv4l is used for all device node accesses.
> >
> 
> Interesting, how does it switch between the modes?

In the File menu there is an entry "Open device" (uses libv4l) and "Open raw device"
(opens the device node directly).

Depending on how the device is opened, all calls to the device node either go through
libv4l or are direct system calls.

> Mauro were you using wrapped mode
> or raw mode when you saw this ?

The button on the toolbar will always use libv4l. But if you add the device node on
the command line (e.g. qv4l2 /dev/video0), then it will open the device node in raw
mode. That's rather inconsistent and it should also use libv4l. I've just fixed
this: 'qv4l2 /dev/video0' will now also use libv4l. If you want to test a device node
bypassing libv4l, then you have to open the device node using "Open raw device" in
the File menu.

I wonder if Mauro got confused by the different behavior as well.

Regards,

	Hans

-- 
Hans Verkuil - video4linux developer - sponsored by Cisco

Re: nasty bug at qv4l2

[Mauro Carvalho Chehab]

Em 24-12-2010 16:54, Hans Verkuil escreveu:
> On Friday, December 24, 2010 15:41:10 Hans de Goede wrote:
>> Hi,
>>
>> On 12/24/2010 03:20 PM, Hans Verkuil wrote:
>>> On Friday, December 24, 2010 15:19:26 Hans de Goede wrote:
>>>> Hi,
>>>>
>>>> On 12/22/2010 12:30 PM, Mauro Carvalho Chehab wrote:
>>>>> Hans V/Hans G,
>>>>>
>>>>> There's a nasty bug at qv4l2 or at libv4l: it is not properly updating
>>>>> all info, if you change the video device. On my tests with uvcvideo (video0)
>>>>> and a gspca camera (pac7302, video1), it was showing the supported formats
>>>>> for the uvcvideo camera when I changed from video0 to video1.
>>>>>
>>>>> The net result is that the image were handled with the wrong decoder
>>>>> (instead of using fourcc V4L2_PIX_FMT_PJPG, it were using BGR3), producing
>>>>> a wrong decoding.
>>>>>
>>>>> Could you please take a look on it?
>>>>
>>>> I'm pretty sure this is not a libv4l issue (other apps which allows witching
>>>> the source work fine), but rather a qv4l2 problem, esp. as it uses libv4lconvert
>>>> directly rather then going through libv4l (iirc).
>>>
>>> And I'm pretty sure it isn't a qv4l2 issue :-)
>>>
>>> For the record: qv4l2 can open a device node either in 'raw' mode bypassing libv4l
>>> and using v4lconvert to convert unsupported pixformats, or in 'wrapped' mode where
>>> libv4l is used for all device node accesses.
>>>
>>
>> Interesting, how does it switch between the modes?
> 
> In the File menu there is an entry "Open device" (uses libv4l) and "Open raw device"
> (opens the device node directly).
> 
> Depending on how the device is opened, all calls to the device node either go through
> libv4l or are direct system calls.
> 
>> Mauro were you using wrapped mode
>> or raw mode when you saw this ?
> 
> The button on the toolbar will always use libv4l. But if you add the device node on
> the command line (e.g. qv4l2 /dev/video0), then it will open the device node in raw
> mode. That's rather inconsistent and it should also use libv4l. I've just fixed
> this: 'qv4l2 /dev/video0' will now also use libv4l. If you want to test a device node
> bypassing libv4l, then you have to open the device node using "Open raw device" in
> the File menu.

Maybe you may add a "--raw" parameter for the command line call.
> 
> I wonder if Mauro got confused by the different behavior as well.

I think I used the libv4l way. I'll re-try on both modes. This way, we'll know for sure if
the issue is at libv4l or not.

Cheers,
Mauro

Re: nasty bug at qv4l2

[Mauro Carvalho Chehab]

Em 25-12-2010 07:14, Mauro Carvalho Chehab escreveu:
> Em 24-12-2010 16:54, Hans Verkuil escreveu:
>> On Friday, December 24, 2010 15:41:10 Hans de Goede wrote:
>>> Hi,
>>>
>>> On 12/24/2010 03:20 PM, Hans Verkuil wrote:
>>>> On Friday, December 24, 2010 15:19:26 Hans de Goede wrote:
>>>>> Hi,
>>>>>
>>>>> On 12/22/2010 12:30 PM, Mauro Carvalho Chehab wrote:
>>>>>> Hans V/Hans G,
>>>>>>
>>>>>> There's a nasty bug at qv4l2 or at libv4l: it is not properly updating
>>>>>> all info, if you change the video device. On my tests with uvcvideo (video0)
>>>>>> and a gspca camera (pac7302, video1), it was showing the supported formats
>>>>>> for the uvcvideo camera when I changed from video0 to video1.
>>>>>>
>>>>>> The net result is that the image were handled with the wrong decoder
>>>>>> (instead of using fourcc V4L2_PIX_FMT_PJPG, it were using BGR3), producing
>>>>>> a wrong decoding.
>>>>>>
>>>>>> Could you please take a look on it?

<snip>

>> I wonder if Mauro got confused by the different behavior as well.
> 
> I think I used the libv4l way. I'll re-try on both modes. This way, we'll know for sure if
> the issue is at libv4l or not.

Double checked: when opening in raw mode, everything works fine. However, when
opening with "libv4l" mode, it doesn't update the supported formats.

Cheers,
Mauro

Re: nasty bug at qv4l2

[Hans de Goede]

Hi,

On 12/25/2010 03:09 PM, Mauro Carvalho Chehab wrote:
> Em 25-12-2010 07:14, Mauro Carvalho Chehab escreveu:
>> Em 24-12-2010 16:54, Hans Verkuil escreveu:
>>> On Friday, December 24, 2010 15:41:10 Hans de Goede wrote:
>>>> Hi,
>>>>
>>>> On 12/24/2010 03:20 PM, Hans Verkuil wrote:
>>>>> On Friday, December 24, 2010 15:19:26 Hans de Goede wrote:
>>>>>> Hi,
>>>>>>
>>>>>> On 12/22/2010 12:30 PM, Mauro Carvalho Chehab wrote:
>>>>>>> Hans V/Hans G,
>>>>>>>
>>>>>>> There's a nasty bug at qv4l2 or at libv4l: it is not properly updating
>>>>>>> all info, if you change the video device. On my tests with uvcvideo (video0)
>>>>>>> and a gspca camera (pac7302, video1), it was showing the supported formats
>>>>>>> for the uvcvideo camera when I changed from video0 to video1.
>>>>>>>
>>>>>>> The net result is that the image were handled with the wrong decoder
>>>>>>> (instead of using fourcc V4L2_PIX_FMT_PJPG, it were using BGR3), producing
>>>>>>> a wrong decoding.
>>>>>>>
>>>>>>> Could you please take a look on it?
>
> <snip>
>
>>> I wonder if Mauro got confused by the different behavior as well.
>>
>> I think I used the libv4l way. I'll re-try on both modes. This way, we'll know for sure if
>> the issue is at libv4l or not.
>
> Double checked: when opening in raw mode, everything works fine. However, when
> opening with "libv4l" mode, it doesn't update the supported formats.
>

I've spend some time looking at this, with interesting results. There is a bug
in libv4lconvert, which gets exposed when using qv4l2 with a pac7311 camera
(no need to first open another type of device).

As suspected, qv4l2 tries to call into libv4lconvert directly even when going
through libv4l. So what happens is:
1) qv4l2 sees rgb24 as a format supported by the device and selects it
    (because of libv4l2 being used)
2) qv4l2 still tries to use libv4lconvert directly and ends up setting up
    conversion from rgb24 to rgb24 (the conversion from pjpg to rgb24 is
    already done transparently by libv4l)
3) libv4lconvert (or rather libv4lcontrol which is part of libv4lconvert)
    enables software whitebalancing by default on pac7311 cameras

libv4lconvert has special code to detect src_fmt == dest_fmt and just do
a memcpy, however this code does not trigger because of 3 (as when doing
processing just a memcpy is not what we want). However libv4lconvert
API is based on providing a src and destination buffer, and in this case
libv4lconvert_convert ends up skipping all steps (conversion, cropping,
flipping and rotating) except for processing, and the processing code
works by modifying the buffer it is passed rather then using a separate
input output buffer, so since none of the other separate input output
buffer needing steps where done, the data never gets copied to the
destination buffer!

I've just pushed a patch to v4l-utils git fixing this bug.

Note that qv4l2 should still be fixed to not call libv4lconvert directly
when not in raw mode (instead it should just do a s_fmt rgb24, which
libv4l will always offer for all supported devices), as the way things
are now libv4lconvert_convert ends up getting called twice. Which will
lead to various software processing steps being done twice, which
is not a problem for whitebalance, but it makes software vflip and
hflip no-ops. And for cameras which need 90 degrees rotation (pac7302)
it will completely screw up the image.

In general an app can either use libv4lconvert directly, or call
libv4l2 functions and let it deal with handling conversion transparently
using both at the same time is not supported.

Regards,

Hans

Re: nasty bug at qv4l2

[Hans Verkuil]

On Tuesday, December 28, 2010 23:53:08 Hans de Goede wrote:
> Hi,
> 
> On 12/25/2010 03:09 PM, Mauro Carvalho Chehab wrote:
> > Em 25-12-2010 07:14, Mauro Carvalho Chehab escreveu:
> >> Em 24-12-2010 16:54, Hans Verkuil escreveu:
> >>> On Friday, December 24, 2010 15:41:10 Hans de Goede wrote:
> >>>> Hi,
> >>>>
> >>>> On 12/24/2010 03:20 PM, Hans Verkuil wrote:
> >>>>> On Friday, December 24, 2010 15:19:26 Hans de Goede wrote:
> >>>>>> Hi,
> >>>>>>
> >>>>>> On 12/22/2010 12:30 PM, Mauro Carvalho Chehab wrote:
> >>>>>>> Hans V/Hans G,
> >>>>>>>
> >>>>>>> There's a nasty bug at qv4l2 or at libv4l: it is not properly updating
> >>>>>>> all info, if you change the video device. On my tests with uvcvideo (video0)
> >>>>>>> and a gspca camera (pac7302, video1), it was showing the supported formats
> >>>>>>> for the uvcvideo camera when I changed from video0 to video1.
> >>>>>>>
> >>>>>>> The net result is that the image were handled with the wrong decoder
> >>>>>>> (instead of using fourcc V4L2_PIX_FMT_PJPG, it were using BGR3), producing
> >>>>>>> a wrong decoding.
> >>>>>>>
> >>>>>>> Could you please take a look on it?
> >
> > <snip>
> >
> >>> I wonder if Mauro got confused by the different behavior as well.
> >>
> >> I think I used the libv4l way. I'll re-try on both modes. This way, we'll know for sure if
> >> the issue is at libv4l or not.
> >
> > Double checked: when opening in raw mode, everything works fine. However, when
> > opening with "libv4l" mode, it doesn't update the supported formats.
> >
> 
> I've spend some time looking at this, with interesting results. There is a bug
> in libv4lconvert, which gets exposed when using qv4l2 with a pac7311 camera
> (no need to first open another type of device).
> 
> As suspected, qv4l2 tries to call into libv4lconvert directly even when going
> through libv4l. So what happens is:
> 1) qv4l2 sees rgb24 as a format supported by the device and selects it
>     (because of libv4l2 being used)
> 2) qv4l2 still tries to use libv4lconvert directly and ends up setting up
>     conversion from rgb24 to rgb24 (the conversion from pjpg to rgb24 is
>     already done transparently by libv4l)
> 3) libv4lconvert (or rather libv4lcontrol which is part of libv4lconvert)
>     enables software whitebalancing by default on pac7311 cameras
> 
> libv4lconvert has special code to detect src_fmt == dest_fmt and just do
> a memcpy, however this code does not trigger because of 3 (as when doing
> processing just a memcpy is not what we want). However libv4lconvert
> API is based on providing a src and destination buffer, and in this case
> libv4lconvert_convert ends up skipping all steps (conversion, cropping,
> flipping and rotating) except for processing, and the processing code
> works by modifying the buffer it is passed rather then using a separate
> input output buffer, so since none of the other separate input output
> buffer needing steps where done, the data never gets copied to the
> destination buffer!
> 
> I've just pushed a patch to v4l-utils git fixing this bug.
> 
> Note that qv4l2 should still be fixed to not call libv4lconvert directly
> when not in raw mode (instead it should just do a s_fmt rgb24, which
> libv4l will always offer for all supported devices), as the way things
> are now libv4lconvert_convert ends up getting called twice. Which will
> lead to various software processing steps being done twice, which
> is not a problem for whitebalance, but it makes software vflip and
> hflip no-ops. And for cameras which need 90 degrees rotation (pac7302)
> it will completely screw up the image.
> 
> In general an app can either use libv4lconvert directly, or call
> libv4l2 functions and let it deal with handling conversion transparently
> using both at the same time is not supported.

Makes a lot of sense.

I've made a patch for qv4l2 that should fix this. It's included below, but
before I push this I'd like to have some test feedback. I could only test with
ivtv and not with any webcams since I don't have access to them at the moment.

So let me know if this fixes qv4l2 and if so, I'll push the patch.

Regards,

	Hans

>From 9e66659528c5aec7ca7bfd1ec881fe4a921e3c7c Mon Sep 17 00:00:00 2001
Message-Id: <9e66659528c5aec7ca7bfd1ec881fe4a921e3c7c.1293647806.git.hverkuil@xs4all.nl>
From: Hans Verkuil <hverkuil@xs4all.nl>
Date: Wed, 29 Dec 2010 19:33:31 +0100
Subject: [PATCH] qv4l2: don't use v4lconvert directly when using libv4l.
To: linux-media@vger.kernel.org

Only in 'raw' mode should v4lconvert be used, otherwise it will be
called twice causing all software image processing to be done twice
as well.

Signed-off-by: Hans Verkuil <hverkuil@xs4all.nl>
---
 utils/qv4l2/qv4l2.cpp |   34 ++++++++++++++++++++++++++--------
 1 files changed, 26 insertions(+), 8 deletions(-)

diff --git a/utils/qv4l2/qv4l2.cpp b/utils/qv4l2/qv4l2.cpp
index 5646bcf..a085f86 100644
--- a/utils/qv4l2/qv4l2.cpp
+++ b/utils/qv4l2/qv4l2.cpp
@@ -178,7 +178,10 @@ void ApplicationWindow::capFrame()
 	switch (m_capMethod) {
 	case methodRead:
 		s = read(m_frameData, m_capSrcFormat.fmt.pix.sizeimage);
-		err = v4lconvert_convert(m_convertData, &m_capSrcFormat, &m_capDestFormat,
+		if (useWrapper())
+			memcpy(m_capImage->bits(), m_frameData, m_capSrcFormat.fmt.pix.sizeimage);
+		else
+			err = v4lconvert_convert(m_convertData, &m_capSrcFormat, &m_capDestFormat,
 				m_frameData, m_capSrcFormat.fmt.pix.sizeimage,
 				m_capImage->bits(), m_capDestFormat.fmt.pix.sizeimage);
 		break;
@@ -190,7 +193,11 @@ void ApplicationWindow::capFrame()
 			return;
 		}
 
-		err = v4lconvert_convert(m_convertData, &m_capSrcFormat, &m_capDestFormat,
+		if (useWrapper())
+			memcpy(m_capImage->bits(), (unsigned char *)m_buffers[buf.index].start,
+					m_capSrcFormat.fmt.pix.sizeimage);
+		else
+			err = v4lconvert_convert(m_convertData, &m_capSrcFormat, &m_capDestFormat,
 				(unsigned char *)m_buffers[buf.index].start, buf.bytesused,
 				m_capImage->bits(), m_capDestFormat.fmt.pix.sizeimage);
 
@@ -209,7 +216,11 @@ void ApplicationWindow::capFrame()
 					&& buf.length == m_buffers[i].length)
 				break;
 
-		err = v4lconvert_convert(m_convertData, &m_capSrcFormat, &m_capDestFormat,
+		if (useWrapper())
+			memcpy(m_capImage->bits(), (unsigned char *)buf.m.userptr,
+					m_capSrcFormat.fmt.pix.sizeimage);
+		else
+			err = v4lconvert_convert(m_convertData, &m_capSrcFormat, &m_capDestFormat,
 				(unsigned char *)buf.m.userptr, buf.bytesused,
 				m_capImage->bits(), m_capDestFormat.fmt.pix.sizeimage);
 
@@ -410,6 +421,11 @@ void ApplicationWindow::capStart(bool start)
 	}
 	m_capMethod = m_genTab->capMethod();
 	g_fmt_cap(m_capSrcFormat);
+	if (useWrapper()) {
+		m_capSrcFormat.fmt.pix.pixelformat = V4L2_PIX_FMT_RGB24;
+		s_fmt(m_capSrcFormat);
+		g_fmt_cap(m_capSrcFormat);
+	}
 	m_frameData = new unsigned char[m_capSrcFormat.fmt.pix.sizeimage];
 	m_capDestFormat = m_capSrcFormat;
 	m_capDestFormat.fmt.pix.pixelformat = V4L2_PIX_FMT_RGB24;
@@ -421,11 +437,13 @@ void ApplicationWindow::capStart(bool start)
 			break;
 		}
 	}
-	v4lconvert_try_format(m_convertData, &m_capDestFormat, &m_capSrcFormat);
-	// v4lconvert_try_format sometimes modifies the source format if it thinks
-	// that there is a better format available. Restore our selected source
-	// format since we do not want that happening.
-	g_fmt_cap(m_capSrcFormat);
+	if (!useWrapper()) {
+		v4lconvert_try_format(m_convertData, &m_capDestFormat, &m_capSrcFormat);
+		// v4lconvert_try_format sometimes modifies the source format if it thinks
+		// that there is a better format available. Restore our selected source
+		// format since we do not want that happening.
+		g_fmt_cap(m_capSrcFormat);
+	}
 	m_capture->setMinimumSize(m_capDestFormat.fmt.pix.width, m_capDestFormat.fmt.pix.height);
 	m_capImage = new QImage(m_capDestFormat.fmt.pix.width, m_capDestFormat.fmt.pix.height, dstFmt);
 	m_capImage->fill(0);
-- 
1.6.4.2



-- 
Hans Verkuil - video4linux developer - sponsored by Cisco

Re: nasty bug at qv4l2

[Mauro Carvalho Chehab]

Em 24-12-2010 12:41, Hans de Goede escreveu:
> Hi,
> 
> On 12/24/2010 03:20 PM, Hans Verkuil wrote:
>> On Friday, December 24, 2010 15:19:26 Hans de Goede wrote:
>>> Hi,
>>>
>>> On 12/22/2010 12:30 PM, Mauro Carvalho Chehab wrote:
>>>> Hans V/Hans G,
>>>>
>>>> There's a nasty bug at qv4l2 or at libv4l: it is not properly updating
>>>> all info, if you change the video device. On my tests with uvcvideo (video0)
>>>> and a gspca camera (pac7302, video1), it was showing the supported formats
>>>> for the uvcvideo camera when I changed from video0 to video1.
>>>>
>>>> The net result is that the image were handled with the wrong decoder
>>>> (instead of using fourcc V4L2_PIX_FMT_PJPG, it were using BGR3), producing
>>>> a wrong decoding.
>>>>
>>>> Could you please take a look on it?
>>>
>>> I'm pretty sure this is not a libv4l issue (other apps which allows witching
>>> the source work fine), but rather a qv4l2 problem, esp. as it uses libv4lconvert
>>> directly rather then going through libv4l (iirc).
>>
>> And I'm pretty sure it isn't a qv4l2 issue :-)
>>
>> For the record: qv4l2 can open a device node either in 'raw' mode bypassing libv4l
>> and using v4lconvert to convert unsupported pixformats, or in 'wrapped' mode where
>> libv4l is used for all device node accesses.
>>
> 
> Interesting, how does it switch between the modes? Mauro were you using wrapped mode
> or raw mode when you saw this ?

I have no idea, as I also didn't know about this feature, nor how to switch from one
mode to the other ;)
> 
> Regards,
> 
> hans

Re: nasty bug at qv4l2

[Hans de Goede]

[-- Attachment #1: Type: text/plain, Size: 756 bytes --]

Hi,

Hans V. I've tested your patch for avoiding the double
conversion problem, and I can report it fixes the issue seen with
webcameras which need 90 degrees rotation.

While testing I found a bug in gspca, which gets triggered by
qv4l2 which makes it impossible to switch between userptr and
mmap mode. While fixing that I also found some locking issues in
gspca. As these all touch the gscpa core I'll send a patch set
to Jean Francois Moine for this.

With the issues in gspca fixed, I found a bug in qv4l2 when using
read mode in raw mode (not passing the correct src_size to
libv4lconvert_convert).

I've attached 2 patches to qv4l2, fixing the read issue and a similar
issue in mmap / userptr mode. These apply on top of your patch.

Regards,

Hans

[-- Attachment #2: 0001-qv4l2-Check-and-use-result-of-read.patch --]
[-- Type: text/plain, Size: 1808 bytes --]

>From d3393364292441fca894186c406a7e9ee982d243 Mon Sep 17 00:00:00 2001
From: Hans de Goede <hdegoede@redhat.com>
Date: Fri, 31 Dec 2010 11:50:28 +0100
Subject: [PATCH 1/2] qv4l2: Check and use result of read()

qv4l2 was calling libv4lconvert_convert with a src size of fmt.pix.sizeimage,
rather then using the actual amount of bytes read. This causes decompressors
which check if they have consumed the entire compressed frame (ie pjpg) to
error out, because they were not being passed the actual frame size.

This patch fixes this, and also adds reporting of libv4lconvert_convert
errors.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 utils/qv4l2/qv4l2.cpp |   14 ++++++++++++--
 1 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/utils/qv4l2/qv4l2.cpp b/utils/qv4l2/qv4l2.cpp
index a085f86..1876b3c 100644
--- a/utils/qv4l2/qv4l2.cpp
+++ b/utils/qv4l2/qv4l2.cpp
@@ -178,11 +178,18 @@ void ApplicationWindow::capFrame()
 	switch (m_capMethod) {
 	case methodRead:
 		s = read(m_frameData, m_capSrcFormat.fmt.pix.sizeimage);
+		if (s < 0) {
+			if (errno != EAGAIN) {
+				error("read");
+				m_capStartAct->setChecked(false);
+			}
+			return;
+		}
 		if (useWrapper())
-			memcpy(m_capImage->bits(), m_frameData, m_capSrcFormat.fmt.pix.sizeimage);
+			memcpy(m_capImage->bits(), m_frameData, s);
 		else
 			err = v4lconvert_convert(m_convertData, &m_capSrcFormat, &m_capDestFormat,
-				m_frameData, m_capSrcFormat.fmt.pix.sizeimage,
+				m_frameData, s,
 				m_capImage->bits(), m_capDestFormat.fmt.pix.sizeimage);
 		break;
 
@@ -227,6 +234,9 @@ void ApplicationWindow::capFrame()
 		qbuf(buf);
 		break;
 	}
+	if (err == -1)
+		error(v4lconvert_get_error_message(m_convertData));
+
 	m_capture->setImage(*m_capImage);
 	if (m_capture->frame() == 1)
 		refresh();
-- 
1.7.3.2


[-- Attachment #3: 0002-qv4l2-When-in-wrapped-mode-memcpy-the-actual-framesi.patch --]
[-- Type: text/plain, Size: 1241 bytes --]

>From 4a3587e7466274f74d89a6608999887f3c62e66a Mon Sep 17 00:00:00 2001
From: Hans de Goede <hdegoede@redhat.com>
Date: Fri, 31 Dec 2010 11:55:44 +0100
Subject: [PATCH 2/2] qv4l2: When in wrapped mode memcpy the actual framesize

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 utils/qv4l2/qv4l2.cpp |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/utils/qv4l2/qv4l2.cpp b/utils/qv4l2/qv4l2.cpp
index 1876b3c..bd1db3e 100644
--- a/utils/qv4l2/qv4l2.cpp
+++ b/utils/qv4l2/qv4l2.cpp
@@ -202,7 +202,7 @@ void ApplicationWindow::capFrame()
 
 		if (useWrapper())
 			memcpy(m_capImage->bits(), (unsigned char *)m_buffers[buf.index].start,
-					m_capSrcFormat.fmt.pix.sizeimage);
+					buf.bytesused);
 		else
 			err = v4lconvert_convert(m_convertData, &m_capSrcFormat, &m_capDestFormat,
 				(unsigned char *)m_buffers[buf.index].start, buf.bytesused,
@@ -225,7 +225,7 @@ void ApplicationWindow::capFrame()
 
 		if (useWrapper())
 			memcpy(m_capImage->bits(), (unsigned char *)buf.m.userptr,
-					m_capSrcFormat.fmt.pix.sizeimage);
+					buf.bytesused);
 		else
 			err = v4lconvert_convert(m_convertData, &m_capSrcFormat, &m_capDestFormat,
 				(unsigned char *)buf.m.userptr, buf.bytesused,
-- 
1.7.3.2

Re: nasty bug at qv4l2

[Hans Verkuil]

On Friday, December 31, 2010 16:02:10 Hans de Goede wrote:
> Hi,
> 
> Hans V. I've tested your patch for avoiding the double
> conversion problem, and I can report it fixes the issue seen with
> webcameras which need 90 degrees rotation.
> 
> While testing I found a bug in gspca, which gets triggered by
> qv4l2 which makes it impossible to switch between userptr and
> mmap mode. While fixing that I also found some locking issues in
> gspca. As these all touch the gscpa core I'll send a patch set
> to Jean Francois Moine for this.
> 
> With the issues in gspca fixed, I found a bug in qv4l2 when using
> read mode in raw mode (not passing the correct src_size to
> libv4lconvert_convert).
> 
> I've attached 2 patches to qv4l2, fixing the read issue and a similar
> issue in mmap / userptr mode. These apply on top of your patch.

Thanks for testing this! I've committed mine and your patches for qv4l2.

Regards,

	Hans

-- 
Hans Verkuil - video4linux developer - sponsored by Cisco

Re: nasty bug at qv4l2

[Mauro Carvalho Chehab]

Em 31-12-2010 13:08, Hans Verkuil escreveu:
> On Friday, December 31, 2010 16:02:10 Hans de Goede wrote:
>> Hi,
>>
>> Hans V. I've tested your patch for avoiding the double
>> conversion problem, and I can report it fixes the issue seen with
>> webcameras which need 90 degrees rotation.
>>
>> While testing I found a bug in gspca, which gets triggered by
>> qv4l2 which makes it impossible to switch between userptr and
>> mmap mode. While fixing that I also found some locking issues in
>> gspca. As these all touch the gscpa core I'll send a patch set
>> to Jean Francois Moine for this.
>>
>> With the issues in gspca fixed, I found a bug in qv4l2 when using
>> read mode in raw mode (not passing the correct src_size to
>> libv4lconvert_convert).
>>
>> I've attached 2 patches to qv4l2, fixing the read issue and a similar
>> issue in mmap / userptr mode. These apply on top of your patch.
> 
> Thanks for testing this! I've committed mine and your patches for qv4l2.

Ok, I did some tests here too. The applied patches fix the bugs I've
reported: the pac7302 driver formats are properly reported (the emulated ones),
and it is now possible to change resolution/format with the uvc driver.

Cheers,
Mauro

[PATCH] Adds the Lego Bionicle to existing sq905c

[Theodore Kilgore]

This patch adds the Vendor:Product number of the Lego Bionicle camera to
the existing gspca/sq905c.c and also a line for the camera in gspca.txt.
The camera works "out of the box" with these small changes. So this is 
just in time for Christmas. Think of the children.

Signed-off-by: Theodore Kilgore <kilgota@auburn.edu>

---------------------------------------------
diff --git a/Documentation/video4linux/gspca.txt b/Documentation/video4linux/gspca.txt
index 6a562ee..261776e 100644
--- a/Documentation/video4linux/gspca.txt
+++ b/Documentation/video4linux/gspca.txt
@@ -366,6 +366,7 @@ t613		17a1:0128	TASCORP JPEG Webcam, NGS Cyclops
 vc032x		17ef:4802	Lenovo Vc0323+MI1310_SOC
 pac207		2001:f115	D-Link DSB-C120
 sq905c		2770:9050	Disney pix micro (CIF)
+sq905c		2770:9051	Lego Bionicle
 sq905c		2770:9052	Disney pix micro 2 (VGA)
 sq905c		2770:905c	All 11 known cameras with this ID
 sq905		2770:9120	All 24 known cameras with this ID
diff --git a/drivers/media/video/gspca/sq905c.c b/drivers/media/video/gspca/sq905c.c
index c2e88b5..8ba1995 100644
--- a/drivers/media/video/gspca/sq905c.c
+++ b/drivers/media/video/gspca/sq905c.c
@@ -301,6 +301,7 @@ static int sd_start(struct gspca_dev *gspca_dev)
 static const __devinitdata struct usb_device_id device_table[] = {
 	{USB_DEVICE(0x2770, 0x905c)},
 	{USB_DEVICE(0x2770, 0x9050)},
+	{USB_DEVICE(0x2770, 0x9051)},
 	{USB_DEVICE(0x2770, 0x9052)},
 	{USB_DEVICE(0x2770, 0x913d)},
 	{}

Re: [PATCH] Adds the Lego Bionicle to existing sq905c

[Hans de Goede]

Mauro,

Will you pick up this patch directly or should I put it in my tree ?

Regards,

Hans


On 12/24/2010 09:06 PM, Theodore Kilgore wrote:
> This patch adds the Vendor:Product number of the Lego Bionicle camera to
> the existing gspca/sq905c.c and also a line for the camera in gspca.txt.
> The camera works "out of the box" with these small changes. So this is
> just in time for Christmas. Think of the children.
>
> Signed-off-by: Theodore Kilgore<kilgota@auburn.edu>
>
> ---------------------------------------------
> diff --git a/Documentation/video4linux/gspca.txt b/Documentation/video4linux/gspca.txt
> index 6a562ee..261776e 100644
> --- a/Documentation/video4linux/gspca.txt
> +++ b/Documentation/video4linux/gspca.txt
> @@ -366,6 +366,7 @@ t613		17a1:0128	TASCORP JPEG Webcam, NGS Cyclops
>   vc032x		17ef:4802	Lenovo Vc0323+MI1310_SOC
>   pac207		2001:f115	D-Link DSB-C120
>   sq905c		2770:9050	Disney pix micro (CIF)
> +sq905c		2770:9051	Lego Bionicle
>   sq905c		2770:9052	Disney pix micro 2 (VGA)
>   sq905c		2770:905c	All 11 known cameras with this ID
>   sq905		2770:9120	All 24 known cameras with this ID
> diff --git a/drivers/media/video/gspca/sq905c.c b/drivers/media/video/gspca/sq905c.c
> index c2e88b5..8ba1995 100644
> --- a/drivers/media/video/gspca/sq905c.c
> +++ b/drivers/media/video/gspca/sq905c.c
> @@ -301,6 +301,7 @@ static int sd_start(struct gspca_dev *gspca_dev)
>   static const __devinitdata struct usb_device_id device_table[] = {
>   	{USB_DEVICE(0x2770, 0x905c)},
>   	{USB_DEVICE(0x2770, 0x9050)},
> +	{USB_DEVICE(0x2770, 0x9051)},
>   	{USB_DEVICE(0x2770, 0x9052)},
>   	{USB_DEVICE(0x2770, 0x913d)},
>   	{}
>

Re: [PATCH] Adds the Lego Bionicle to existing sq905c

[Mauro Carvalho Chehab]

Em 24-12-2010 17:55, Hans de Goede escreveu:
> Mauro,
> 
> Will you pick up this patch directly or should I put it in my tree ?

Either way works for me (but I prefer if one of the gspca maintainers/sub-mainainers
pick). If you don't pick it, please reply with your ack.
> 
> Regards,
> 
> Hans
> 
> 
> On 12/24/2010 09:06 PM, Theodore Kilgore wrote:
>> This patch adds the Vendor:Product number of the Lego Bionicle camera to
>> the existing gspca/sq905c.c and also a line for the camera in gspca.txt.
>> The camera works "out of the box" with these small changes. So this is
>> just in time for Christmas. Think of the children.
>>
>> Signed-off-by: Theodore Kilgore<kilgota@auburn.edu>
>>
>> ---------------------------------------------
>> diff --git a/Documentation/video4linux/gspca.txt b/Documentation/video4linux/gspca.txt
>> index 6a562ee..261776e 100644
>> --- a/Documentation/video4linux/gspca.txt
>> +++ b/Documentation/video4linux/gspca.txt
>> @@ -366,6 +366,7 @@ t613        17a1:0128    TASCORP JPEG Webcam, NGS Cyclops
>>   vc032x        17ef:4802    Lenovo Vc0323+MI1310_SOC
>>   pac207        2001:f115    D-Link DSB-C120
>>   sq905c        2770:9050    Disney pix micro (CIF)
>> +sq905c        2770:9051    Lego Bionicle
>>   sq905c        2770:9052    Disney pix micro 2 (VGA)
>>   sq905c        2770:905c    All 11 known cameras with this ID
>>   sq905        2770:9120    All 24 known cameras with this ID
>> diff --git a/drivers/media/video/gspca/sq905c.c b/drivers/media/video/gspca/sq905c.c
>> index c2e88b5..8ba1995 100644
>> --- a/drivers/media/video/gspca/sq905c.c
>> +++ b/drivers/media/video/gspca/sq905c.c
>> @@ -301,6 +301,7 @@ static int sd_start(struct gspca_dev *gspca_dev)
>>   static const __devinitdata struct usb_device_id device_table[] = {
>>       {USB_DEVICE(0x2770, 0x905c)},
>>       {USB_DEVICE(0x2770, 0x9050)},
>> +    {USB_DEVICE(0x2770, 0x9051)},
>>       {USB_DEVICE(0x2770, 0x9052)},
>>       {USB_DEVICE(0x2770, 0x913d)},
>>       {}
>>

Re: [PATCH] Adds the Lego Bionicle to existing sq905c

[Hans de Goede]

Hi,

On 12/25/2010 10:20 AM, Mauro Carvalho Chehab wrote:
> Em 24-12-2010 17:55, Hans de Goede escreveu:
>> Mauro,
>>
>> Will you pick up this patch directly or should I put it in my tree ?
>
> Either way works for me (but I prefer if one of the gspca maintainers/sub-mainainers
> pick). If you don't pick it, please reply with your ack.

Given that nothing else is going into my tree at the moment I would
prefer for you to pick it up directly, so:

Acked-by: Hans de Goede <hdegoede@redhat.com>

Thanks & Regards,

Hans


>>
>> Regards,
>>
>> Hans
>>
>>
>> On 12/24/2010 09:06 PM, Theodore Kilgore wrote:
>>> This patch adds the Vendor:Product number of the Lego Bionicle camera to
>>> the existing gspca/sq905c.c and also a line for the camera in gspca.txt.
>>> The camera works "out of the box" with these small changes. So this is
>>> just in time for Christmas. Think of the children.
>>>
>>> Signed-off-by: Theodore Kilgore<kilgota@auburn.edu>
>>>
>>> ---------------------------------------------
>>> diff --git a/Documentation/video4linux/gspca.txt b/Documentation/video4linux/gspca.txt
>>> index 6a562ee..261776e 100644
>>> --- a/Documentation/video4linux/gspca.txt
>>> +++ b/Documentation/video4linux/gspca.txt
>>> @@ -366,6 +366,7 @@ t613        17a1:0128    TASCORP JPEG Webcam, NGS Cyclops
>>>    vc032x        17ef:4802    Lenovo Vc0323+MI1310_SOC
>>>    pac207        2001:f115    D-Link DSB-C120
>>>    sq905c        2770:9050    Disney pix micro (CIF)
>>> +sq905c        2770:9051    Lego Bionicle
>>>    sq905c        2770:9052    Disney pix micro 2 (VGA)
>>>    sq905c        2770:905c    All 11 known cameras with this ID
>>>    sq905        2770:9120    All 24 known cameras with this ID
>>> diff --git a/drivers/media/video/gspca/sq905c.c b/drivers/media/video/gspca/sq905c.c
>>> index c2e88b5..8ba1995 100644
>>> --- a/drivers/media/video/gspca/sq905c.c
>>> +++ b/drivers/media/video/gspca/sq905c.c
>>> @@ -301,6 +301,7 @@ static int sd_start(struct gspca_dev *gspca_dev)
>>>    static const __devinitdata struct usb_device_id device_table[] = {
>>>        {USB_DEVICE(0x2770, 0x905c)},
>>>        {USB_DEVICE(0x2770, 0x9050)},
>>> +    {USB_DEVICE(0x2770, 0x9051)},
>>>        {USB_DEVICE(0x2770, 0x9052)},
>>>        {USB_DEVICE(0x2770, 0x913d)},
>>>        {}
>>>
>

Re: [PATCH] Adds the Lego Bionicle to existing sq905c

[Jean-Francois Moine]

On Sat, 25 Dec 2010 10:36:20 +0100
Hans de Goede <hdegoede@redhat.com> wrote:
> On 12/25/2010 10:20 AM, Mauro Carvalho Chehab wrote:
> > Em 24-12-2010 17:55, Hans de Goede escreveu:
> >> Mauro,
> >>
> >> Will you pick up this patch directly or should I put it in my
> >> tree ?
> >
> > Either way works for me (but I prefer if one of the gspca
> > maintainers/sub-mainainers pick). If you don't pick it, please
> > reply with your ack.
> 
> Given that nothing else is going into my tree at the moment I would
> prefer for you to pick it up directly, so:
> 
> Acked-by: Hans de Goede <hdegoede@redhat.com>

Hi Mauro and Hans,

As I have some changes to do in this driver, I may also apply
Theodore's patch.

Cheers.

-- 
Ken ar c'hentañ	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/

Re: [PATCH] Adds the Lego Bionicle to existing sq905c

[Mauro Carvalho Chehab]

Em 25-12-2010 07:52, Jean-Francois Moine escreveu:
> On Sat, 25 Dec 2010 10:36:20 +0100
> Hans de Goede <hdegoede@redhat.com> wrote:
>> On 12/25/2010 10:20 AM, Mauro Carvalho Chehab wrote:
>>> Em 24-12-2010 17:55, Hans de Goede escreveu:
>>>> Mauro,
>>>>
>>>> Will you pick up this patch directly or should I put it in my
>>>> tree ?
>>>
>>> Either way works for me (but I prefer if one of the gspca
>>> maintainers/sub-mainainers pick). If you don't pick it, please
>>> reply with your ack.
>>
>> Given that nothing else is going into my tree at the moment I would
>> prefer for you to pick it up directly, so:
>>
>> Acked-by: Hans de Goede <hdegoede@redhat.com>
> 
> Hi Mauro and Hans,
> 
> As I have some changes to do in this driver, I may also apply
> Theodore's patch.

Ok, thanks!
> 
> Cheers.
> 

Re: [PATCH] Adds the Lego Bionicle to existing sq905c

[Theodore Kilgore]

On Sat, 25 Dec 2010, Jean-Francois Moine wrote:

> On Sat, 25 Dec 2010 10:36:20 +0100
> Hans de Goede <hdegoede@redhat.com> wrote:
> > On 12/25/2010 10:20 AM, Mauro Carvalho Chehab wrote:
> > > Em 24-12-2010 17:55, Hans de Goede escreveu:
> > >> Mauro,
> > >>
> > >> Will you pick up this patch directly or should I put it in my
> > >> tree ?
> > >
> > > Either way works for me (but I prefer if one of the gspca
> > > maintainers/sub-mainainers pick). If you don't pick it, please
> > > reply with your ack.
> > 
> > Given that nothing else is going into my tree at the moment I would
> > prefer for you to pick it up directly, so:
> > 
> > Acked-by: Hans de Goede <hdegoede@redhat.com>
> 
> Hi Mauro and Hans,
> 
> As I have some changes to do in this driver, 

...

Jean-Francois, 

If you do not mind, tell me more about the "some changes." More cameras? 
Or something else? 

Someone in the family has been quite sick for the last several months, 
which has taken a lot of my time and energy and may do so in the future, 
too. I have not had the time to do much more than to look at the mail I 
get every day, much less to continue work on related projects. Several 
things that I wanted to do months ago are sitting incomplete and in limbo 
waiting for me to be less busy and preoccupied.

But I have not lost interest.

Wishing for you and for all of us a peaceful and prosperous year 2011.

Theodore Kilgore

Re: [PATCH] Adds the Lego Bionicle to existing sq905c

[Jean-Francois Moine]

On Sat, 25 Dec 2010 12:14:25 -0600 (CST)
Theodore Kilgore <kilgota@banach.math.auburn.edu> wrote:

> > As I have some changes to do in this driver,   
>  
> If you do not mind, tell me more about the "some changes." More
> cameras? Or something else? 

Hi Theodore,

Sorry, I made a mistake. I had changes for the sq930x instead...

The best also for you and for everybody for 2011.

-- 
Ken ar c'hentañ	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/

Re: [PATCH] Adds the Lego Bionicle to existing sq905c

[Theodore Kilgore]

On Sat, 25 Dec 2010, Hans de Goede wrote:

> Hi,
> 
> On 12/25/2010 10:20 AM, Mauro Carvalho Chehab wrote:
> > Em 24-12-2010 17:55, Hans de Goede escreveu:
> > > Mauro,
> > > 
> > > Will you pick up this patch directly or should I put it in my tree ?
> > 
> > Either way works for me (but I prefer if one of the gspca
> > maintainers/sub-mainainers
> > pick). If you don't pick it, please reply with your ack.
> 
> Given that nothing else is going into my tree at the moment I would
> prefer for you to pick it up directly, so:
> 
> Acked-by: Hans de Goede <hdegoede@redhat.com>
> 
> Thanks & Regards,
> 
> Hans

And thank you.

Theodore Kilgore