Re: [PATCH] em28xx: Fix height setting on non-progressive captures
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 > --- > 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 Signed-off-by: Mauro Carvalho Chehab 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 -- 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
Re: [PATCH] em28xx: Fix height setting on non-progressive captures
Em 04-08-2012 05:53, llar...@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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] em28xx: Fix height setting on non-progressive captures
> >> 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
--
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
Re: [PATCH] em28xx: Fix height setting on non-progressive captures
On Fri, Aug 3, 2012 at 3:55 PM, Devin Heitmueller
wrote:
> On Fri, Aug 3, 2012 at 2:42 PM, Ezequiel Garcia 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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] em28xx: Fix height setting on non-progressive captures
On Fri, Aug 3, 2012 at 2:42 PM, Ezequiel Garcia 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
Re: [PATCH] em28xx: Fix height setting on non-progressive captures
Hi Devin,
Thanks for answering.
On Fri, Aug 3, 2012 at 3:26 PM, Devin Heitmueller
wrote:
> On Fri, Aug 3, 2012 at 2:11 PM, Ezequiel Garcia wrote:
>> On Fri, Aug 3, 2012 at 2:52 PM, Ezequiel Garcia
>> 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.
--
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
Re: [PATCH] em28xx: Fix height setting on non-progressive captures
On Fri, Aug 3, 2012 at 2:11 PM, Ezequiel Garcia wrote: > On Fri, Aug 3, 2012 at 2:52 PM, Ezequiel Garcia 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 -- 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
Re: [PATCH] em28xx: Fix height setting on non-progressive captures
On Fri, Aug 3, 2012 at 2:52 PM, Ezequiel Garcia 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 -- 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
