[PATCH] em28xx: Fix height setting on non-progressive captures

[PATCH] em28xx: Fix height setting on non-progressive captures

[Ezequiel Garcia]

This was introduced on commit c2a6b54a9:
"em28xx: fix: don't do image interlacing on webcams"
It is a known bug that has already been reported several times
and confirmed by Mauro.
Tested by compilation only.

Signed-off-by: Ezequiel Garcia <elezegarcia@gmail.com>
---
Hi,

I have no idea why this hasn't been fixed before.

See this mail for Mauro's confirmation
http://www.digipedia.pl/usenet/thread/18550/7691/#post7685
where he requested a patch on reporter. 

I guess the patch never came in.

Regards,
Ezequiel.
---
 drivers/media/video/em28xx/em28xx-core.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/media/video/em28xx/em28xx-core.c b/drivers/media/video/em28xx/em28xx-core.c
index de2cb20..6daa861 100644
--- a/drivers/media/video/em28xx/em28xx-core.c
+++ b/drivers/media/video/em28xx/em28xx-core.c
@@ -786,7 +786,7 @@ int em28xx_resolution_set(struct em28xx *dev)
 		dev->vbi_height = 18;
 
 	if (!dev->progressive)
-		height >>= norm_maxh(dev);
+		height = height / 2;
 
 	em28xx_set_outfmt(dev);
 
-- 
1.7.4.4

Re: [PATCH] em28xx: Fix height setting on non-progressive captures

[Ezequiel Garcia]

On Fri, Aug 3, 2012 at 2:52 PM, Ezequiel Garcia <elezegarcia@gmail.com> wrote:
> This was introduced on commit c2a6b54a9:
> "em28xx: fix: don't do image interlacing on webcams"
> It is a known bug that has already been reported several times
> and confirmed by Mauro.
> Tested by compilation only.
>

I wonder if it's possible to get an Ack or a Tested-By from any of the
em28xx owners?

Also, Devin: you mentioned in an old mail [1] you had some patches for em28xx,
but you had no time to put them into shape for submission.

If you want to, send then to me (or the full em28xx tree) and I can
try to submit
the patches.

Thanks,
Ezequiel.

[1] http://www.mail-archive.com/linux-media@vger.kernel.org/msg48232.html

Re: [PATCH] em28xx: Fix height setting on non-progressive captures

[Devin Heitmueller]

On Fri, Aug 3, 2012 at 2:11 PM, Ezequiel Garcia <elezegarcia@gmail.com> wrote:
> On Fri, Aug 3, 2012 at 2:52 PM, Ezequiel Garcia <elezegarcia@gmail.com> wrote:
>> This was introduced on commit c2a6b54a9:
>> "em28xx: fix: don't do image interlacing on webcams"
>> It is a known bug that has already been reported several times
>> and confirmed by Mauro.
>> Tested by compilation only.
>>
>
> I wonder if it's possible to get an Ack or a Tested-By from any of the
> em28xx owners?

This shouldn't be accepted upstream without testing at least on x86.
I did make such a change to make it work in my ARM tree, but I don't
fully understand the nature of the change and I'm not completely
confident it's correct for x86 (based on my reading of the datasheet
and how the accumulator field is structured in the em28xx chip).
Also, I actually don't have any progressive devices (I've got probably
a dozen em28xx devices, but they are all interlaced capture), which
made me particularly hesitant to submit this patch.

> Also, Devin: you mentioned in an old mail [1] you had some patches for em28xx,
> but you had no time to put them into shape for submission.
>
> If you want to, send then to me (or the full em28xx tree) and I can
> try to submit
> the patches.

Yeah, probably not a bad idea.  I've been sitting on the tree because
they haven't been tested on any other platforms and some of them are
not necessarily generally suitable for the mainline kernel.  And of
course the tree needs to be parsed out into an actual patch series,
and each patch has to be individually validated across multiple
devices to ensure they don't cause breakage (they were tested on an
em2863, but I have no idea if they cause problems on other chips such
as the em2820 or em2880).

All that said, I'm not really sure what the benefit would be in
sending you the tree if you don't actually have any hardware to test
with.  The last thing we need is more crap being sent upstream that is
"compile tested only" since that's where many of the regressions come
from (well meaning people sending completely untested 'cleanup
patches' can cause more harm than good).

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com

Re: [PATCH] em28xx: Fix height setting on non-progressive captures

[Ezequiel Garcia]

Hi Devin,

Thanks for answering.

On Fri, Aug 3, 2012 at 3:26 PM, Devin Heitmueller
<dheitmueller@kernellabs.com> wrote:
> On Fri, Aug 3, 2012 at 2:11 PM, Ezequiel Garcia <elezegarcia@gmail.com> wrote:
>> On Fri, Aug 3, 2012 at 2:52 PM, Ezequiel Garcia <elezegarcia@gmail.com> wrote:
>>> This was introduced on commit c2a6b54a9:
>>> "em28xx: fix: don't do image interlacing on webcams"
>>> It is a known bug that has already been reported several times
>>> and confirmed by Mauro.
>>> Tested by compilation only.
>>>
>>
>> I wonder if it's possible to get an Ack or a Tested-By from any of the
>> em28xx owners?
>
> This shouldn't be accepted upstream without testing at least on x86.
> I did make such a change to make it work in my ARM tree, but I don't
> fully understand the nature of the change and I'm not completely
> confident it's correct for x86 (based on my reading of the datasheet
> and how the accumulator field is structured in the em28xx chip).
> Also, I actually don't have any progressive devices (I've got probably
> a dozen em28xx devices, but they are all interlaced capture), which
> made me particularly hesitant to submit this patch.
>

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.



>> Also, Devin: you mentioned in an old mail [1] you had some patches for em28xx,
>> but you had no time to put them into shape for submission.
>>
>> If you want to, send then to me (or the full em28xx tree) and I can
>> try to submit
>> the patches.
>
> Yeah, probably not a bad idea.  I've been sitting on the tree because
> they haven't been tested on any other platforms and some of them are
> not necessarily generally suitable for the mainline kernel.  And of
> course the tree needs to be parsed out into an actual patch series,
> and each patch has to be individually validated across multiple
> devices to ensure they don't cause breakage (they were tested on an
> em2863, but I have no idea if they cause problems on other chips such
> as the em2820 or em2880).
>
> All that said, I'm not really sure what the benefit would be in
> sending you the tree if you don't actually have any hardware to test
> with.  The last thing we need is more crap being sent upstream that is
> "compile tested only" since that's where many of the regressions come
> from (well meaning people sending completely untested 'cleanup
> patches' can cause more harm than good).
>

Mmm, you're right. I was rather thinking in patches that fixes "obvious"
(whatever that may be) things and assuming these patches could get some
community testing.

So: never mind, bad idea. I've sent enough zero-test patches, which
only means more work for Mauro :-(

Thanks,
Ezequiel.

Re: [PATCH] em28xx: Fix height setting on non-progressive captures

[Devin Heitmueller]

On Fri, Aug 3, 2012 at 2:42 PM, Ezequiel Garcia <elezegarcia@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

Re: [PATCH] em28xx: Fix height setting on non-progressive captures

[Ezequiel Garcia]

On Fri, Aug 3, 2012 at 3:55 PM, Devin Heitmueller
<dheitmueller@kernellabs.com> wrote:
> On Fri, Aug 3, 2012 at 2:42 PM, Ezequiel Garcia <elezegarcia@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.
>

I agree with you. It's very odd that is working as it is.

As a final word, I believe you confused this patch affecting
progressive capture,
when actually it only affects non-progressive (interlaced) capture devices,
so perhaps you could give it a try yourself.

That is: *if* I got you right, and *if* you're not too busy.

Thanks,
Ezequiel.

Re: [PATCH] em28xx: Fix height setting on non-progressive captures

[llarevo]

> >> 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.

I own a Terratec Cinergy XS USB in two flavors:  0ccd:005e and
0ccd:0042. I work with  Fedora F17. If somebody gives me an advice what
code to patch (git or a tarball from
http://linuxtv.org/downloads/drivers/) and what to test, I can make a
try.

Regards
-- 
Felix

Re: [PATCH] em28xx: Fix height setting on non-progressive captures

[Mauro Carvalho Chehab]

Em 04-08-2012 05:53, llarevo@gmx.net escreveu:
>>>> 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.
> 
> I own a Terratec Cinergy XS USB in two flavors:  0ccd:005e and
> 0ccd:0042. I work with  Fedora F17. If somebody gives me an advice what
> code to patch (git or a tarball from
> http://linuxtv.org/downloads/drivers/) and what to test, I can make a
> try.

Thanks for your offering, but this should affect only em28xx-based
webcams (like the Silvercrest one).

I have a few here. I'll do the testing.

Regards,
Mauro

Re: [PATCH] em28xx: Fix height setting on non-progressive captures

[Mauro Carvalho Chehab]

Em 03-08-2012 14:52, Ezequiel Garcia escreveu:
> This was introduced on commit c2a6b54a9:
> "em28xx: fix: don't do image interlacing on webcams"
> It is a known bug that has already been reported several times
> and confirmed by Mauro.

Thanks for reminding us about that.

> Tested by compilation only.
> 
> Signed-off-by: Ezequiel Garcia <elezegarcia@gmail.com>
> ---
> Hi,
> 
> I have no idea why this hasn't been fixed before.

The reason was because that patch didn't work ;)

> 
> See this mail for Mauro's confirmation
> http://www.digipedia.pl/usenet/thread/18550/7691/#post7685
> where he requested a patch on reporter. 
> 
> I guess the patch never came in.


Did some tests here with both TV and Webcam (progressive)
devices. The enclosed patch fixes the issue.

Regards,
Mauro.

[media] em28xx: Fix height setting on non-progressive captures

This was introduced on commit c2a6b54a9:
"em28xx: fix: don't do image interlacing on webcams"

The proposed patch by Ezequiel is wrong. The right fix here is to just
don't bother here if either the image is progressive or not.

Reported-by: Ezequiel Garcia <elezegarcia@gmail.com>
Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>

diff --git a/drivers/media/video/em28xx/em28xx-core.c b/drivers/media/video/em28xx/em28xx-core.c
index de2cb20..bed07a6 100644
--- a/drivers/media/video/em28xx/em28xx-core.c
+++ b/drivers/media/video/em28xx/em28xx-core.c
@@ -785,12 +785,8 @@ int em28xx_resolution_set(struct em28xx *dev)
 	else
 		dev->vbi_height = 18;
 
-	if (!dev->progressive)
-		height >>= norm_maxh(dev);
-
 	em28xx_set_outfmt(dev);
 
-
 	em28xx_accumulator_set(dev, 1, (width - 4) >> 2, 1, (height - 4) >> 2);
 
 	/* If we don't set the start position to 2 in VBI mode, we end up