nasty bug at qv4l2

PATCH In review Mauro Carvalho Chehab 5 participants 22 messages Dec 22, 2010 – Dec 31, 2010

Subsystems (2)

Documentation Media (V4L/DVB)

lore

Share & tools

lore

Mauro Carvalho Chehab

author

2010-12-22 11:31 UTC #0 lore raw

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

Hans de Goede

2010-12-24 14:13 UTC #1 lore raw

Hi,

Quoted 7 lines · 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.

Quoted 5 lines

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

Hans Verkuil

2010-12-24 14:20 UTC #2 lore raw

Quoted 14 lines · On Friday, December 24, 2010 15:19:26 Hans de Goede wrote:

Hi,
Quoted 11 lines · 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.

Quoted 5 lines

Quoted 1 line
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.

Quoted 1 line

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

Signature · 1 line

Hans Verkuil - video4linux developer - sponsored by Cisco

Hans de Goede

2010-12-24 14:35 UTC #3 lore raw

Hi,

Quoted 21 lines · Hans Verkuil

Quoted 14 lines · On Friday, December 24, 2010 15:19:26 Hans de Goede wrote:
Hi,
Quoted 11 lines · 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.
Quoted 5 lines
Quoted 1 line
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).

Quoted 6 lines

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

Hans Verkuil

2010-12-24 18:55 UTC #4 lore raw

Quoted 24 lines · On Friday, December 24, 2010 15:41:10 Hans de Goede wrote:

Hi,
Quoted 21 lines · Hans Verkuil
Quoted 19 lines · On Friday, December 24, 2010 15:19:26 Hans de Goede wrote:
Hi,
Quoted 7 lines · 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.
Quoted 5 lines
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).

Quoted 8 lines

Quoted 6 lines
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.

Quoted 2 lines

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

Signature · 1 line

Hans Verkuil - video4linux developer - sponsored by Cisco

Mauro Carvalho Chehab

author

2010-12-25 09:14 UTC #5 lore raw

Em 24-12-2010 16:54, Hans Verkuil escreveu:

Quoted 43 lines · On Friday, December 24, 2010 15:41:10 Hans de Goede wrote:

Quoted 24 lines
Hi,
Quoted 21 lines · Hans Verkuil
Quoted 19 lines · On Friday, December 24, 2010 15:19:26 Hans de Goede wrote:
Hi,
Quoted 7 lines · 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.
Quoted 5 lines
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).
Quoted 8 lines
Quoted 6 lines
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.
Quoted 2 lines
Mauro were you using wrapped mode
or raw mode when you saw this ?

Quoted 6 lines

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.

Quoted 2 lines

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

Mauro Carvalho Chehab

author

2010-12-25 14:09 UTC #6 lore raw

Em 25-12-2010 07:14, Mauro Carvalho Chehab escreveu:

Quoted 21 lines

Em 24-12-2010 16:54, Hans Verkuil escreveu:
Quoted 20 lines · On Friday, December 24, 2010 15:41:10 Hans de Goede wrote:
Quoted 19 lines
Hi,
Quoted 16 lines · Hans Verkuil
Quoted 15 lines · On Friday, December 24, 2010 15:19:26 Hans de Goede wrote:
Hi,
Quoted 12 lines · 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?

<snip>

Quoted 4 lines

Quoted 1 line
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

Hans de Goede

2010-12-28 22:46 UTC #7 lore raw

Hi,

Quoted 25 lines · Mauro Carvalho Chehab

Em 25-12-2010 07:14, Mauro Carvalho Chehab escreveu:
Quoted 21 lines
Em 24-12-2010 16:54, Hans Verkuil escreveu:
Quoted 20 lines · On Friday, December 24, 2010 15:41:10 Hans de Goede wrote:
Quoted 19 lines
Hi,
Quoted 16 lines · Hans Verkuil
Quoted 15 lines · On Friday, December 24, 2010 15:19:26 Hans de Goede wrote:
Hi,
Quoted 12 lines · 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?
<snip>

Quoted 8 lines

Quoted 4 lines
Quoted 1 line
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

Hans Verkuil

2010-12-29 18:41 UTC #8 lore raw

Quoted 73 lines · On Tuesday, December 28, 2010 23:53:08 Hans de Goede wrote:

Hi,
Quoted 25 lines · Mauro Carvalho Chehab
Em 25-12-2010 07:14, Mauro Carvalho Chehab escreveu:
Quoted 21 lines
Em 24-12-2010 16:54, Hans Verkuil escreveu:
Quoted 20 lines · On Friday, December 24, 2010 15:41:10 Hans de Goede wrote:
Quoted 19 lines
Hi,
Quoted 16 lines · Hans Verkuil
Quoted 15 lines · On Friday, December 24, 2010 15:19:26 Hans de Goede wrote:
Hi,
Quoted 12 lines · 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?
<snip>
Quoted 8 lines
Quoted 4 lines
Quoted 1 line
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.

Quoted 3 lines

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

Quoted 1 line

From 9e66659528c5aec7ca7bfd1ec881fe4a921e3c7c Mon Sep 17 00:00:00 2001

Message-Id: <9e66659528c5aec7ca7bfd1ec881fe4a921e3c7c.1293647806.git.hverkuil@xs4all.nl>
From: Hans Verkuil <[email protected]>
Date: Wed, 29 Dec 2010 19:33:31 +0100
Subject: [PATCH] qv4l2: don't use v4lconvert directly when using libv4l.
To: [email protected]

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 <[email protected]>

---
 utils/qv4l2/qv4l2.cpp |   34 ++++++++++++++++++++++++++--------
 1 files changed, 26 insertions(+), 8 deletions(-)

utils/qv4l2/qv4l2.cpp +26 -8

index 5646bcf..a085f86 100644
@@ -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);

Signature · 6 lines

1.6.4.2



-- 
Hans Verkuil - video4linux developer - sponsored by Cisco

Mauro Carvalho Chehab

author

2010-12-25 08:54 UTC #9 lore raw

In reply to Hans de Goede

Em 24-12-2010 12:41, Hans de Goede escreveu:

Quoted 26 lines

Hi,
Quoted 23 lines · Hans Verkuil
Quoted 14 lines · On Friday, December 24, 2010 15:19:26 Hans de Goede wrote:
Hi,
Quoted 11 lines · 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.
Quoted 5 lines
Quoted 1 line
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 :-)

Quoted 7 lines

Quoted 4 lines
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 ;)

Quoted 4 lines

Regards,
hans

Hans de Goede

2010-12-31 14:55 UTC #10 lore raw

In reply to Hans Verkuil

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

Quoted 1 line

From d3393364292441fca894186c406a7e9ee982d243 Mon Sep 17 00:00:00 2001

From: Hans de Goede <[email protected]>
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 <[email protected]>

---
 utils/qv4l2/qv4l2.cpp |   14 ++++++++++++--
 1 files changed, 12 insertions(+), 2 deletions(-)

utils/qv4l2/qv4l2.cpp +12 -2

index a085f86..1876b3c 100644
@@ -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();

Signature · 1 line

1.7.3.2

Quoted 1 line

From 4a3587e7466274f74d89a6608999887f3c62e66a Mon Sep 17 00:00:00 2001

From: Hans de Goede <[email protected]>
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 <[email protected]>

---
 utils/qv4l2/qv4l2.cpp |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

utils/qv4l2/qv4l2.cpp +2 -2

index 1876b3c..bd1db3e 100644
@@ -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,

Signature · 1 line

1.7.3.2

Hans Verkuil

2010-12-31 15:08 UTC #11 lore raw

Quoted 12 lines · 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.

Quoted 6 lines

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

Signature · 1 line

Hans Verkuil - video4linux developer - sponsored by Cisco

Mauro Carvalho Chehab

author

2010-12-31 16:18 UTC #12 lore raw

Em 31-12-2010 13:08, Hans Verkuil escreveu:

Quoted 13 lines · On Friday, December 31, 2010 16:02:10 Hans de Goede wrote:

Quoted 12 lines
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.

Quoted 8 lines

Quoted 6 lines
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

Theodore Kilgore

2010-12-24 19:29 UTC #13 lore raw

PATCH 1/1 Adds the Lego Bionicle to existing sq905c

In reply to Hans de Goede

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 <[email protected]>

---------------------------------------------

Documentation/video4linux/gspca.txt +1 -0

index 6a562ee..261776e 100644
@@ -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

drivers/media/video/gspca/sq905c.c +1 -0

index c2e88b5..8ba1995 100644
@@ -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)},
 	{}

Hans de Goede

2010-12-24 19:49 UTC #14 lore raw

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

Mauro,

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

Regards,

Hans

Quoted 33 lines · 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<[email protected]>
---------------------------------------------
Documentation/video4linux/gspca.txt +1 -0
index 6a562ee..261776e 100644
@@ -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
drivers/media/video/gspca/sq905c.c +1 -0
index c2e88b5..8ba1995 100644
@@ -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)},
  	{}

Mauro Carvalho Chehab

author

2010-12-25 09:20 UTC #15 lore raw

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

Em 24-12-2010 17:55, Hans de Goede escreveu:

Quoted 3 lines

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.

Quoted 40 lines

Regards,
Hans
Quoted 33 lines · 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<[email protected]>
---------------------------------------------
Documentation/video4linux/gspca.txt +1 -0
index 6a562ee..261776e 100644
@@ -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
drivers/media/video/gspca/sq905c.c +1 -0
index c2e88b5..8ba1995 100644
@@ -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)},
      {}

Hans de Goede

Acked-by

2010-12-25 09:30 UTC #16 lore raw

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

Hi,

Quoted 7 lines · Mauro Carvalho Chehab

Em 24-12-2010 17:55, Hans de Goede escreveu:
Quoted 3 lines
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 <[email protected]>

Thanks & Regards,

Hans

Quoted 41 lines

Quoted 40 lines
Regards,
Hans
Quoted 33 lines · 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<[email protected]>
---------------------------------------------
Documentation/video4linux/gspca.txt +1 -0
index 6a562ee..261776e 100644
@@ -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
drivers/media/video/gspca/sq905c.c +1 -0
index c2e88b5..8ba1995 100644
@@ -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)},
       {}

Jean-Francois Moine

2010-12-25 09:50 UTC #17 lore raw

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

Quoted 7 lines · On Sat, 25 Dec 2010 10:36:20 +0100 Hans de Goede <[email protected]> wrote:

Quoted 6 lines · Mauro Carvalho Chehab
Em 24-12-2010 17:55, Hans de Goede escreveu:
Quoted 4 lines
Mauro,
Will you pick up this patch directly or should I put it in my tree ?

Quoted 8 lines

Quoted 3 lines
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 <[email protected]>

Hi Mauro and Hans,

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

Cheers.

Signature · 2 lines

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

Mauro Carvalho Chehab

author

2010-12-25 10:24 UTC #18 lore raw

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

Em 25-12-2010 07:52, Jean-Francois Moine escreveu:

Quoted 16 lines

Quoted 14 lines · On Sat, 25 Dec 2010 10:36:20 +0100 Hans de Goede <[email protected]> wrote:
Quoted 9 lines · Mauro Carvalho Chehab
Em 24-12-2010 17:55, Hans de Goede escreveu:
Quoted 4 lines
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:

Quoted 6 lines

Quoted 1 line
Acked-by Hans de Goede <[email protected]>
Hi Mauro and Hans,
As I have some changes to do in this driver, I may also apply Theodore's patch.

Ok, thanks!

Quoted 3 lines

Cheers.

Theodore Kilgore

2010-12-25 17:38 UTC #19 lore raw

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

In reply to Jean-Francois Moine

Quoted 13 lines · Jean-Francois Moine

Quoted 11 lines · On Sat, 25 Dec 2010 10:36:20 +0100 Hans de Goede <[email protected]> wrote:
Quoted 9 lines · Mauro Carvalho Chehab
Em 24-12-2010 17:55, Hans de Goede escreveu:
Quoted 4 lines
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.

Quoted 8 lines

Quoted 4 lines
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 <[email protected]>
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

Jean-Francois Moine

2010-12-25 18:42 UTC #20 lore raw

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

Quoted 4 lines · On Sat, 25 Dec 2010 12:14:25 -0600 (CST) Theodore Kilgore…

Quoted 1 line
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.

Signature · 2 lines

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

Theodore Kilgore

2010-12-25 17:23 UTC #21 lore raw

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

In reply to Hans de Goede

Quoted 12 lines · Hans de Goede

Hi,
Quoted 8 lines · Mauro Carvalho Chehab
Em 24-12-2010 17:55, Hans de Goede escreveu:
Quoted 3 lines
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.

Quoted 8 lines

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 <[email protected]>
Thanks & Regards,
Hans

And thank you.

Theodore Kilgore