Am 30.01.2013 10:49, schrieb Hans Verkuil: > On Wed 30 January 2013 10:40:30 Mauro Carvalho Chehab wrote: >> Em Wed, 30 Jan 2013 09:01:22 +0100 >> Hans Verkuil <hverkuil@xxxxxxxxx> 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 <hans.verkuil@xxxxxxxxx> >>> --- >>> 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 > depth=16 -> bytesperline = 1440 > > By rounding the bytesperline for depth = 12 would become 1440, which is wrong. Isn't the actual problem that the bytesperline value is calculated based on the depth instead of size of the largest plane ? While this formula gives us the right value for YUV411P (12bit), the calculated value for YUV422P (same plane size but 16bit depth) is wrong. I see two possibilities to solve this: 1) use switch/case(format_type) in vidioc_g_fmt_vid_cap() and vidioc_try_fmt_vid_cap() 2) add plane size to format data struct em28xx_fmt (either as total or relative value) I prefer 2) to keep things together and to make adding new format easier. Could you please also fix static void em28xx_copy_video() - int bytesperline = dev->width << 1; + int bytesperline = (dev->width * dev->format->depth) >> 8 AFAICS, bytesperline here is supposed to describe the actual number of bytes used per line (in opposition to g/try_fmt). Regards, Frank > >>> + f->fmt.pix.sizeimage = (dev->width * dev->height * dev->format->depth + 7) >> 3; >>> f->fmt.pix.colorspace = V4L2_COLORSPACE_SMPTE170M; >>> >>> /* FIXME: TOP? NONE? BOTTOM? ALTENATE? */ >>> @@ -906,8 +906,8 @@ static int vidioc_try_fmt_vid_cap(struct file *file, void *priv, >>> f->fmt.pix.width = width; >>> f->fmt.pix.height = height; >>> f->fmt.pix.pixelformat = fmt->fourcc; >>> - f->fmt.pix.bytesperline = (dev->width * fmt->depth + 7) >> 3; >>> - f->fmt.pix.sizeimage = f->fmt.pix.bytesperline * height; >>> + f->fmt.pix.bytesperline = width * (fmt->depth >> 3); >> Why did you remove the round up here? > See above. > > Regards, > > Hans > >>> + f->fmt.pix.sizeimage = (width * height * fmt->depth + 7) >> 3; >>> f->fmt.pix.colorspace = V4L2_COLORSPACE_SMPTE170M; >>> if (dev->progressive) >>> f->fmt.pix.field = V4L2_FIELD_NONE; >> >> Regards, >> Mauro >> -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
