diff mbox

em28xx: Fix height setting on non-progressive captures

Message ID CALF0-+Vhng3=GUJs5k9fiktkE6mEtDNEzKfP8+zjTSmCCRez8w@mail.gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Ezequiel Garcia Aug. 3, 2012, 6:42 p.m. UTC 
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<--------------------------

  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.
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Devin Heitmueller Aug. 3, 2012, 6:55 p.m. UTC | #1 
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
Ezequiel Garcia Aug. 3, 2012, 7:02 p.m. UTC | #2 
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.
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
llarevo@gmx.net Aug. 4, 2012, 8:53 a.m. UTC | #3 
> >> 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
Mauro Carvalho Chehab Aug. 12, 2012, 8:56 a.m. UTC | #4 
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
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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