On Fri, Aug 3, 2012 at 2:42 PM, Ezequiel Garcia <elezegar...@gmail.com> wrote:
> Wait a minute, unless I completely misunderstood the bug (which is possible),
> I think this patch is straightforward.
>
> By the look of this hunk on commit c2a6b54a:
>
> ---------------------------------8<--------------------------
> diff --git a/drivers/media/video/em28xx/em28xx-core.c
> b/drivers/media/video/em28xx/em28xx-core.c
> index 5b78e19..339fffd 100644
> --- a/drivers/media/video/em28xx/em28xx-core.c
> +++ b/drivers/media/video/em28xx/em28xx-core.c
> @@ -720,7 +720,10 @@ int em28xx_resolution_set(struct em28xx *dev)
> {
> int width, height;
> width = norm_maxw(dev);
> - height = norm_maxh(dev) >> 1;
> + height = norm_maxh(dev);
> +
> + if (!dev->progressive)
> + height >>= norm_maxh(dev);
>
> --------------------------------->8--------------------------
>
> It seems to me that for non-progressive the height should just be
>
> height = height / 2 (or height = height >> 1)
>
> as was before, and as my patch is doing. It seems to driver will
> "merge" the interlaced
> frames and so the "expected" height is half the real height.
> I hope I got it right.
>
> That said and no matter how straightforward may be, which I'm not sure,
> I also want the patch to get tested before being accepted.
So my concern here is that I don't actually know what that code does
on x86 (what does height end up being equal to?). On ARM it results
in height being zero, but on x86 I don't know what it will do (and the
fact that it works on x86 despite the change makes me worry about a
regression being introduced).
In other words, it might be working just out of dumb luck and making
the code behave like it does on ARM may cause problems for devices
other than the one I tested with.
I guess I'm worried that the broken code might be a no-op on x86 and
the height ends up still being 480 (or 576 for PAL), in which case
cutting the size of the accumulator window in half may introduce
problems not being seen before.
Devin
--
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
