From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from mx1.redhat.com ([209.132.183.28]:7847 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754924Ab3A3THh (ORCPT ); Wed, 30 Jan 2013 14:07:37 -0500 Date: Wed, 30 Jan 2013 17:07:29 -0200 From: Mauro Carvalho Chehab To: Hans Verkuil Cc: "linux-media" , Frank =?UTF-8?B?U2Now6Rm?= =?UTF-8?B?ZXI=?= , Devin Heitmueller Subject: Re: [RFCv2 PATCH] em28xx: fix bytesperline calculation in G/TRY_FMT Message-ID: <20130130170729.59d9e04d@redhat.com> In-Reply-To: <201301301049.25541.hverkuil@xs4all.nl> References: <201301300901.22486.hverkuil@xs4all.nl> <20130130074030.455a1185@redhat.com> <201301301049.25541.hverkuil@xs4all.nl> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-media-owner@vger.kernel.org List-ID: Em Wed, 30 Jan 2013 10:49:25 +0100 Hans Verkuil escreveu: > On Wed 30 January 2013 10:40:30 Mauro Carvalho Chehab wrote: > > Em Wed, 30 Jan 2013 09:01:22 +0100 > > Hans Verkuil escreveu: > > > > > This was part of my original em28xx patch series. That particular patch > > > combined two things: this fix and the change where TRY_FMT would no > > > longer return -EINVAL for unsupported pixelformats. The latter change was > > > rejected (correctly), but we all forgot about the second part of the patch > > > which fixed a real bug. I'm reposting just that fix. > > > > > > Changes since v1: > > > > > > - v1 still miscalculated the bytesperline and imagesize values (they were > > > too large). > > > - G_FMT had the same calculation bug. > > > > > > Tested with my em28xx. > > > > > > Regards, > > > > > > Hans > > > > > > The bytesperline calculation was incorrect: it used the old width instead of > > > the provided width in the case of TRY_FMT, and it miscalculated the bytesperline > > > value for the depth == 12 (planar YUV 4:1:1) case. For planar formats the > > > bytesperline value should be the bytesperline of the widest plane, which is > > > the Y plane which has 8 bits per pixel, not 12. > > > > > > Signed-off-by: Hans Verkuil > > > --- > > > drivers/media/usb/em28xx/em28xx-video.c | 8 ++++---- > > > 1 file changed, 4 insertions(+), 4 deletions(-) > > > > > > diff --git a/drivers/media/usb/em28xx/em28xx-video.c b/drivers/media/usb/em28xx/em28xx-video.c > > > index 2eabf2a..6ced426 100644 > > > --- a/drivers/media/usb/em28xx/em28xx-video.c > > > +++ b/drivers/media/usb/em28xx/em28xx-video.c > > > @@ -837,8 +837,8 @@ static int vidioc_g_fmt_vid_cap(struct file *file, void *priv, > > > f->fmt.pix.width = dev->width; > > > f->fmt.pix.height = dev->height; > > > f->fmt.pix.pixelformat = dev->format->fourcc; > > > - f->fmt.pix.bytesperline = (dev->width * dev->format->depth + 7) >> 3; > > > - f->fmt.pix.sizeimage = f->fmt.pix.bytesperline * dev->height; > > > + f->fmt.pix.bytesperline = dev->width * (dev->format->depth >> 3); > > > > Why did you remove the round up here? > > Because that would give the wrong result. Depth can be 8, 12 or 16. The YUV 4:1:1 > planar format is the one with depth 12. But for the purposes of the bytesperline > calculation only the depth of the largest plane counts, which is the luma plane > with a depth of 8. So for a width of 720 the value of bytesperline should be: > > depth=8 -> bytesperline = 720 > depth=12 -> bytesperline = 720 With depth=12, it should be, instead, 1080, as 2 pixels need 3 bytes. > depth=16 -> bytesperline = 1440 Well, depth=8 -> bytesperline = (720 * 8) + 7) / 8 = 720 depth=12 -> bytesperline = (720 * 12) + 7) / 8 = 1080 depth=16 -> bytesperline = (720 * 16) + 7) / 8 = 1440 So, this sounds perfectly OK on my eyes: f->fmt.pix.bytesperline = (dev->width * dev->format->depth + 7) >> 3; Regards, Mauro