Re: [PULL] generic image bounds setting and alignment function

2009-06-17 Thread Trent Piepho
On Wed, 17 Jun 2009, Guennadi Liakhovetski wrote:
> On Tue, 16 Jun 2009, Trent Piepho wrote:
> > On Tue, 16 Jun 2009, Guennadi Liakhovetski wrote:
> > > > 01/14: compat: handle __fls
> > > > http://linuxtv.org/hg/~tap/v4l-dvb?cmd=changeset;node=c4b55ce6c273
> > > >
> > > > 02/14: v4l2: Create helper function for bounding and aligning images
> > > > http://linuxtv.org/hg/~tap/v4l-dvb?cmd=changeset;node=b4d3ec8d363d
> > >
> > > I am sorry, I will not bother with saving, reformatting, pasting... Just
> > > wanted to ask about this place:
> >
> > I guess you do not use Mercurial like all other v4l-dvb developers?
>
> I do use hg, though not for development, but for interacting with "all
> other v4l-dvb developers"
>
> > Because you are making a big deal about a simple operation than can be done
> > with a few keystrokes.  If I wanted this patch quoted in my editor, I can
> > simply type:  !!cd ~/v4l-dvb ; hg export b4d3ec8d363d | sed 's/^/> /'
>
> No, typing this is not a big deal, as you say. But I do not understand
> _wny_ everyone, wishing to review / comment on your patches has to do
> that. And another problem of your approach you confirm yourself in this
> post:

Using pull requests is something all v4l developers, besides you, do as
well.  No one, besides you, seems to find it a problem.  It's been this way
for years.  I'm not the one who came up with this system.  Sometimes one
needs to go the mountain instead of expecting the mountain to come to
oneself.

> See? Now hg will have two patches, which Mauro will then have to merge
> into one when exporting to git, and which then will be very difficult to

Oh well.  It's happened plenty of times before.  I try not to make a habit
of it.

One can find many many patches in the linux git tree that have bugs in
them.  Often there are patches that fix these bugs included in the same
upstream merge.  IOW, the bug was found and fixed before the patch was
merged upstream but the fix wasn't folded into the original patch, because
the original patch was already in git and someone didn't want to do a
git-rebase.  One advantage of the hg tree is we get an extra opportunity to
fix these things before sending them to git.

> everyone gets to see and review your patches only when they are already in
> your external repository ready to be pulled by the maintainer.

s/in your/posted in the/; s/repository/mailing list/; s/pulled/applied/;

everyone gets to see and review your patches only when they are already
posted in the external mailing list ready to be applied by the maintainer.

Is that really any different?
--
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: [PULL] generic image bounds setting and alignment function

2009-06-17 Thread Trent Piepho
On Wed, 17 Jun 2009, Guennadi Liakhovetski wrote:
> On Tue, 16 Jun 2009, Trent Piepho wrote:
>
>   /* up the smaller alignment until we have enough */
>   do {
> - if (walign <= halign && walign < wmaxa) {
> + if (halign >= hmaxa ||
> + (walign <= halign && walign < wmaxa)) {
>
> Do I understand it right now, that as soon as your halign now reaches
> hmaxa, you'll stop incrementing halign and then keep incrementing walign
> even beyond wmaxa?...

Yes.

It's clearly documented that the function isn't designed to handle
impossible constraints.  If both walign and halign are at max and there
still needs to be more alignment then there is no possible resolution, no
matter what width & height started at.

This is just one of many ways the constraints could be impossible.  Max
width being less than min width is another obvious example.  I decided it
wasn't worth complicating the code to check for all of these things.  And
if the function did return failure the callers would need to check that.
None of the code v4l_bound_align_image() replaces has a failure path for
impossible image size constraints, because the constraints aren't
impossible.
--
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: [PULL] generic image bounds setting and alignment function

2009-06-17 Thread Guennadi Liakhovetski
On Tue, 16 Jun 2009, Trent Piepho wrote:

> On Tue, 16 Jun 2009, Mauro Carvalho Chehab wrote:
> > Em Tue, 16 Jun 2009 17:57:20 +0200 (CEST)
> > Guennadi Liakhovetski  escreveu:
> > > On Sat, 30 May 2009, Trent Piepho wrote:
> > > + if (walign + halign < salign) {
> > > + /* Max walign where there is still a valid width */
> > > + unsigned int wmaxa = __fls(wmax ^ (wmin - 1));
> > >
> > > I cannot follow correctness of the above, sorry. Take a simple example:
> > > wmax=0x7f, wmin=7, wmaxa = __fls(0x7f ^ 6) = __fls(0x79) = 0. And the
> > > comment says it's the "maximum walign where there is still a valid width."
> > > What am I missing?
> > >
> > > +
> > > + /* up the smaller alignment until we have enough */
> > > + do {
> > > + if (walign <= halign && walign < wmaxa) {
> > >
> >
> > As I'm still cooking the patches, I prefer to postpone the align ones until 
> > we
> > are comfortable with them.
> >
> > Trent,
> >
> > Could you please take a look on the above comments
> 
> There is no bug with the wmaxa code, but there is a different bug
> elsewhere.
> 
> Please pull from http://linuxtv.org/hg/~tap/fix
> 
> for the following changeset:
> 
> 01/01: v4l2: Fix flaw in alignment code
> http://linuxtv.org/hg/~tap/fix?cmd=changeset;node=4ef7fb102b6c

/* up the smaller alignment until we have enough */
do {
-   if (walign <= halign && walign < wmaxa) {
+   if (halign >= hmaxa ||
+   (walign <= halign && walign < wmaxa)) {

Do I understand it right now, that as soon as your halign now reaches 
hmaxa, you'll stop incrementing halign and then keep incrementing walign 
even beyond wmaxa?...

*w = clamp_align(*w, wmin, wmax, walign + 1);
walign = __ffs(*w);
} else {

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
--
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: [PULL] generic image bounds setting and alignment function

2009-06-16 Thread Guennadi Liakhovetski
On Tue, 16 Jun 2009, Trent Piepho wrote:

> On Tue, 16 Jun 2009, Guennadi Liakhovetski wrote:
> > > 01/14: compat: handle __fls
> > > http://linuxtv.org/hg/~tap/v4l-dvb?cmd=changeset;node=c4b55ce6c273
> > >
> > > 02/14: v4l2: Create helper function for bounding and aligning images
> > > http://linuxtv.org/hg/~tap/v4l-dvb?cmd=changeset;node=b4d3ec8d363d
> >
> > I am sorry, I will not bother with saving, reformatting, pasting... Just
> > wanted to ask about this place:
> 
> I guess you do not use Mercurial like all other v4l-dvb developers?

I do use hg, though not for development, but for interacting with "all 
other v4l-dvb developers"

> Because you are making a big deal about a simple operation than can be done
> with a few keystrokes.  If I wanted this patch quoted in my editor, I can
> simply type:  !!cd ~/v4l-dvb ; hg export b4d3ec8d363d | sed 's/^/> /'

No, typing this is not a big deal, as you say. But I do not understand 
_wny_ everyone, wishing to review / comment on your patches has to do 
that. And another problem of your approach you confirm yourself in this 
post:

> That is less then you wrote to complain how about how hard this is for you.
> 
> > +   if (walign + halign < salign) {
> > +   /* Max walign where there is still a valid width */
> > +   unsigned int wmaxa = __fls(wmax ^ (wmin - 1));
> >
> > I cannot follow correctness of the above, sorry. Take a simple example:
> > wmax=0x7f, wmin=7, wmaxa = __fls(0x7f ^ 6) = __fls(0x79) = 0. And the
> > comment says it's the "maximum walign where there is still a valid width."
> > What am I missing?
> 
> What you are missing is __fls vs __ffs.  __fls() finds the _last_ set bit.
> In this case __fls(0x79) = 6.  Which is correct as 1 << 6 = 0x40, and there
> is a multiple of 0x40 between 0x7f and 7, while there are no multiples of
> the next higher alignment, 0x80, between 0x7f and 7.

Right, indeed, for some reason I swapped fls and ffs in my head. Thanks 
for explaining.

> In a more difficult example, wmax=0x7f and wmin=0x71, the largest alignment
> possible is 1 << 3 == 0x08.  The next higher alignment, 0x10, will not work
> as 0x70 is too small and 0x80 is too large.  __fls(0x7f ^ 0x70) =
> __fls(0x0f) = 3.
> 
> > +
> > +   /* up the smaller alignment until we have enough */
> > +   do {
> > +   if (walign <= halign && walign < wmaxa) {
> 
> Thank you for bringing this code up again, as I now realize there is a bug
> here.  It doesn't check if halign is below hmaxa, or even calculate hmaxa.
> I thought this would be ok because the code isn't designed to handle
> impossible constraints.  But it might increase halign too much when it's
> possible to satisfy the salign requirement by increasing walign.

See? Now hg will have two patches, which Mauro will then have to merge 
into one when exporting to git, and which then will be very difficult to 
trace back from git to its hg origins. Although, once the code is in git, 
who will ever want to look at its hgorigin?... And this happens because 
everyone gets to see and review your patches only when they are already in 
your external repository ready to be pulled by the maintainer.

So, there are two problems here: _how_ your patches have to be reviewed 
and _when_ they get to be seen.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
--
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: [PULL] generic image bounds setting and alignment function

2009-06-16 Thread Trent Piepho
On Tue, 16 Jun 2009, Mauro Carvalho Chehab wrote:
> Em Tue, 16 Jun 2009 17:57:20 +0200 (CEST)
> Guennadi Liakhovetski  escreveu:
> > On Sat, 30 May 2009, Trent Piepho wrote:
> > +   if (walign + halign < salign) {
> > +   /* Max walign where there is still a valid width */
> > +   unsigned int wmaxa = __fls(wmax ^ (wmin - 1));
> >
> > I cannot follow correctness of the above, sorry. Take a simple example:
> > wmax=0x7f, wmin=7, wmaxa = __fls(0x7f ^ 6) = __fls(0x79) = 0. And the
> > comment says it's the "maximum walign where there is still a valid width."
> > What am I missing?
> >
> > +
> > +   /* up the smaller alignment until we have enough */
> > +   do {
> > +   if (walign <= halign && walign < wmaxa) {
> >
>
> As I'm still cooking the patches, I prefer to postpone the align ones until we
> are comfortable with them.
>
> Trent,
>
> Could you please take a look on the above comments

There is no bug with the wmaxa code, but there is a different bug
elsewhere.

Please pull from http://linuxtv.org/hg/~tap/fix

for the following changeset:

01/01: v4l2: Fix flaw in alignment code
http://linuxtv.org/hg/~tap/fix?cmd=changeset;node=4ef7fb102b6c
--
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: [PULL] generic image bounds setting and alignment function

2009-06-16 Thread Trent Piepho
On Tue, 16 Jun 2009, Guennadi Liakhovetski wrote:
> > 01/14: compat: handle __fls
> > http://linuxtv.org/hg/~tap/v4l-dvb?cmd=changeset;node=c4b55ce6c273
> >
> > 02/14: v4l2: Create helper function for bounding and aligning images
> > http://linuxtv.org/hg/~tap/v4l-dvb?cmd=changeset;node=b4d3ec8d363d
>
> I am sorry, I will not bother with saving, reformatting, pasting... Just
> wanted to ask about this place:

I guess you do not use Mercurial like all other v4l-dvb developers?
Because you are making a big deal about a simple operation than can be done
with a few keystrokes.  If I wanted this patch quoted in my editor, I can
simply type:  !!cd ~/v4l-dvb ; hg export b4d3ec8d363d | sed 's/^/> /'

That is less then you wrote to complain how about how hard this is for you.

> + if (walign + halign < salign) {
> + /* Max walign where there is still a valid width */
> + unsigned int wmaxa = __fls(wmax ^ (wmin - 1));
>
> I cannot follow correctness of the above, sorry. Take a simple example:
> wmax=0x7f, wmin=7, wmaxa = __fls(0x7f ^ 6) = __fls(0x79) = 0. And the
> comment says it's the "maximum walign where there is still a valid width."
> What am I missing?

What you are missing is __fls vs __ffs.  __fls() finds the _last_ set bit.
In this case __fls(0x79) = 6.  Which is correct as 1 << 6 = 0x40, and there
is a multiple of 0x40 between 0x7f and 7, while there are no multiples of
the next higher alignment, 0x80, between 0x7f and 7.

In a more difficult example, wmax=0x7f and wmin=0x71, the largest alignment
possible is 1 << 3 == 0x08.  The next higher alignment, 0x10, will not work
as 0x70 is too small and 0x80 is too large.  __fls(0x7f ^ 0x70) =
__fls(0x0f) = 3.

> +
> + /* up the smaller alignment until we have enough */
> + do {
> + if (walign <= halign && walign < wmaxa) {

Thank you for bringing this code up again, as I now realize there is a bug
here.  It doesn't check if halign is below hmaxa, or even calculate hmaxa.
I thought this would be ok because the code isn't designed to handle
impossible constraints.  But it might increase halign too much when it's
possible to satisfy the salign requirement by increasing walign.
--
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: [PULL] generic image bounds setting and alignment function

2009-06-16 Thread Mauro Carvalho Chehab
Em Tue, 16 Jun 2009 17:57:20 +0200 (CEST)
Guennadi Liakhovetski  escreveu:

> On Sat, 30 May 2009, Trent Piepho wrote:
> 
> > Mauro,
> > 
> > Please pull from http://linuxtv.org/hg/~tap/v4l-dvb
> > 
> > This series adds a function for bounding and alignment image sizes and
> > modifies a number of drivers to use it.  It came up when the pxa patches to
> > deal with the alignment issues for that driver were posted.  I haven't
> > tested these patches for pxa.
> > 
> > for the following 14 changesets:
> > 
> > 01/14: compat: handle __fls
> > http://linuxtv.org/hg/~tap/v4l-dvb?cmd=changeset;node=c4b55ce6c273
> > 
> > 02/14: v4l2: Create helper function for bounding and aligning images
> > http://linuxtv.org/hg/~tap/v4l-dvb?cmd=changeset;node=b4d3ec8d363d
> 
> I am sorry, I will not bother with saving, reformatting, pasting... Just 
> wanted to ask about this place:
> 
> + if (walign + halign < salign) {
> + /* Max walign where there is still a valid width */
> + unsigned int wmaxa = __fls(wmax ^ (wmin - 1));
> 
> I cannot follow correctness of the above, sorry. Take a simple example: 
> wmax=0x7f, wmin=7, wmaxa = __fls(0x7f ^ 6) = __fls(0x79) = 0. And the 
> comment says it's the "maximum walign where there is still a valid width." 
> What am I missing?
> 
> +
> + /* up the smaller alignment until we have enough */
> + do {
> + if (walign <= halign && walign < wmaxa) {
> 

As I'm still cooking the patches, I prefer to postpone the align ones until we
are comfortable with them.

Trent,

Could you please take a look on the above comments



Cheers,
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: [PULL] generic image bounds setting and alignment function

2009-06-16 Thread Guennadi Liakhovetski
On Sat, 30 May 2009, Trent Piepho wrote:

> Mauro,
> 
> Please pull from http://linuxtv.org/hg/~tap/v4l-dvb
> 
> This series adds a function for bounding and alignment image sizes and
> modifies a number of drivers to use it.  It came up when the pxa patches to
> deal with the alignment issues for that driver were posted.  I haven't
> tested these patches for pxa.
> 
> for the following 14 changesets:
> 
> 01/14: compat: handle __fls
> http://linuxtv.org/hg/~tap/v4l-dvb?cmd=changeset;node=c4b55ce6c273
> 
> 02/14: v4l2: Create helper function for bounding and aligning images
> http://linuxtv.org/hg/~tap/v4l-dvb?cmd=changeset;node=b4d3ec8d363d

I am sorry, I will not bother with saving, reformatting, pasting... Just 
wanted to ask about this place:

+   if (walign + halign < salign) {
+   /* Max walign where there is still a valid width */
+   unsigned int wmaxa = __fls(wmax ^ (wmin - 1));

I cannot follow correctness of the above, sorry. Take a simple example: 
wmax=0x7f, wmin=7, wmaxa = __fls(0x7f ^ 6) = __fls(0x79) = 0. And the 
comment says it's the "maximum walign where there is still a valid width." 
What am I missing?

+
+   /* up the smaller alignment until we have enough */
+   do {
+   if (walign <= halign && walign < wmaxa) {

> 
> 03/14: pxa-camera: Use v4l bounding/alignment function
> http://linuxtv.org/hg/~tap/v4l-dvb?cmd=changeset;node=cb48209c1841

And this one uses the salign functionality of v4l_bound_align_image(), 
so...

> 
> 04/14: sh_mobile_ceu_camera: Use v4l bounding/alignment function
> http://linuxtv.org/hg/~tap/v4l-dvb?cmd=changeset;node=08dc3db16e9a
> 
> 05/14: zoran: Use v4l bounding/alignment functiob
> http://linuxtv.org/hg/~tap/v4l-dvb?cmd=changeset;node=d65d274ca9da
> 
> 06/14: vivi: Use v4l bounding/alignment function
> http://linuxtv.org/hg/~tap/v4l-dvb?cmd=changeset;node=67a3342606c2
> 
> 07/14: saa7134: Use v4l bounding/alignment function
> http://linuxtv.org/hg/~tap/v4l-dvb?cmd=changeset;node=8e79122888da
> 
> 08/14: cx88: Use v4l bounding/alignment function
> http://linuxtv.org/hg/~tap/v4l-dvb?cmd=changeset;node=7dc2db9c0b34
> 
> 09/14: w8968cf: Use v4l bounding/alignment function
> http://linuxtv.org/hg/~tap/v4l-dvb?cmd=changeset;node=1733bbc12c3a
> 
> 10/14: cx23885: Use v4l bounding/alignment function
> http://linuxtv.org/hg/~tap/v4l-dvb?cmd=changeset;node=605ec680bd75
> 
> 11/14: mt9: Use v4l bounding/alignment function
> http://linuxtv.org/hg/~tap/v4l-dvb?cmd=changeset;node=d92d47e0d76f
> 
> 12/14: cx231xx: Use v4l bounding/alignment function
> http://linuxtv.org/hg/~tap/v4l-dvb?cmd=changeset;node=3da0c824a487
> 
> 13/14: em28xx: Use v4l bounding/alignment function
> http://linuxtv.org/hg/~tap/v4l-dvb?cmd=changeset;node=42b042c376ec
> 
> 14/14: cx231xx: TRY_FMT should not actually set anything
> http://linuxtv.org/hg/~tap/v4l-dvb?cmd=changeset;node=81ca6b823ec4

Thanks for making this easy
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
--
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: [PULL] generic image bounds setting and alignment function

2009-06-12 Thread Robert Jarzmik
Trent Piepho  writes:

> On Mon, 1 Jun 2009, Robert Jarzmik wrote:
>> Trent Piepho  writes:
>> > Please pull from http://linuxtv.org/hg/~tap/v4l-dvb
>> If I'm not mistaken, these lines are an equivalent of :
>>  balign = 1 << align;
>> if (align)
>>  x = ALIGN(x + 1 - balign/2, balign);
>>
>> Isn't that simpler to read ?
>
> I don't think so, it's not obvious that it will round to the nearest value.
> You have to look up ALIGN and then __ALIGN_MASK and see that it will in
> effect add balign - 1 and then reduce x + 1 - balign/2 + balign - 1 into x
> + balign/2.
You're thinking from your code.

>>  x = ALIGN(x + 1 - balign/2, balign);
That means : "take x, add 1, substract half of alignment, and return nearest
aligned value".
IOW, by substracting half of the alignment, you have the guarantee that if
you're at a distance of less half alignment from aligned value N, result will be
N.

> It also generates worse code.  You'd think the compiled would be able to
> optimize it into the same code, but it doesn't.  Unless there is some issue
> with how it will work with negative values that causes a subtle difference.
That's a sensible argument. I presume you checked the assembly here.

I'm still thinking a balign/2 is clearer than (1 << (align - 1)), but well, as
it appears I'm the only one bothered, let's go ahead.

What about the comment codying style comment of pxa_camera ?

-- 
Robert
--
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: [PULL] generic image bounds setting and alignment function

2009-06-06 Thread Guennadi Liakhovetski
Hi Mauro

On Sat, 6 Jun 2009, Mauro Carvalho Chehab wrote:

> Hi Guennadi,
> 
> Em Wed, 3 Jun 2009 20:26:36 +0200 (CEST)
> Guennadi Liakhovetski  escreveu:
> 
> > On Mon, 1 Jun 2009, Robert Jarzmik wrote:
> > 
> > > Trent Piepho  writes:
> > > 
> > > > Mauro,
> > > >
> > > > Please pull from http://linuxtv.org/hg/~tap/v4l-dvb
> > > >
> > > > This series adds a function for bounding and alignment image sizes and
> > > > modifies a number of drivers to use it.  It came up when the pxa 
> > > > patches to
> > > > deal with the alignment issues for that driver were posted.  I haven't
> > > > tested these patches for pxa.
> > > 
> > > Hi Trent,
> > > 
> > > I didn't see the review of that serie, I'm curious what others said.
> > 
> > I strongly agree with Robert. Can we PLEASE have this series posted to the 
> > list for a review first? I don't think a CC in a pull-request suffices.
> 
> All patches added at the -hg staging tree are copied to the commits ML:
>   http://www.linuxtv.org/cgi-bin/mailman/listinfo/linuxtv-commits
> 
> Feel free to review and post a public comment for they. I'll hold those 
> patches
> outside my upstream merging -git tree for now, to give you some time to 
> review.
> Since the merge window for the next kernel is almost opening, we shouldn't 
> have
> much time.

Thanks, but I wasn't subscribed to that list at the time the patches were 
posted, and in any case, AFAIU, linuxtv-commits is an announcement list, 
used to announce patches _after_ they have been applied to the respective 
tree, and not for patch review and discussion _before_ they get applied. 
Please, correct me if I am wrong.

So, unfortunately, that list doesn't help me. Commenting on mails in it is 
about as convenient as commenting on commits from the hg tree.

I would much prefer and would be very grateful if Trent could re-post his 
patches to this (LMML) list for review, I don't think this should be very 
difficult to do.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
--
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: [PULL] generic image bounds setting and alignment function

2009-06-06 Thread Mauro Carvalho Chehab
Hi Guennadi,

Em Wed, 3 Jun 2009 20:26:36 +0200 (CEST)
Guennadi Liakhovetski  escreveu:

> On Mon, 1 Jun 2009, Robert Jarzmik wrote:
> 
> > Trent Piepho  writes:
> > 
> > > Mauro,
> > >
> > > Please pull from http://linuxtv.org/hg/~tap/v4l-dvb
> > >
> > > This series adds a function for bounding and alignment image sizes and
> > > modifies a number of drivers to use it.  It came up when the pxa patches 
> > > to
> > > deal with the alignment issues for that driver were posted.  I haven't
> > > tested these patches for pxa.
> > 
> > Hi Trent,
> > 
> > I didn't see the review of that serie, I'm curious what others said.
> 
> I strongly agree with Robert. Can we PLEASE have this series posted to the 
> list for a review first? I don't think a CC in a pull-request suffices.

All patches added at the -hg staging tree are copied to the commits ML:
http://www.linuxtv.org/cgi-bin/mailman/listinfo/linuxtv-commits

Feel free to review and post a public comment for they. I'll hold those patches
outside my upstream merging -git tree for now, to give you some time to review.
Since the merge window for the next kernel is almost opening, we shouldn't have
much time.



Cheers,
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: [PULL] generic image bounds setting and alignment function

2009-06-03 Thread Guennadi Liakhovetski
On Mon, 1 Jun 2009, Robert Jarzmik wrote:

> Trent Piepho  writes:
> 
> > Mauro,
> >
> > Please pull from http://linuxtv.org/hg/~tap/v4l-dvb
> >
> > This series adds a function for bounding and alignment image sizes and
> > modifies a number of drivers to use it.  It came up when the pxa patches to
> > deal with the alignment issues for that driver were posted.  I haven't
> > tested these patches for pxa.
> 
> Hi Trent,
> 
> I didn't see the review of that serie, I'm curious what others said.

I strongly agree with Robert. Can we PLEASE have this series posted to the 
list for a review first? I don't think a CC in a pull-request suffices.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
--
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: [PULL] generic image bounds setting and alignment function

2009-06-03 Thread Trent Piepho
On Mon, 1 Jun 2009, Robert Jarzmik wrote:
> Trent Piepho  writes:
> > Please pull from http://linuxtv.org/hg/~tap/v4l-dvb
> >
> > This series adds a function for bounding and alignment image sizes and
> > modifies a number of drivers to use it.  It came up when the pxa patches to
> > deal with the alignment issues for that driver were posted.  I haven't
> > tested these patches for pxa.
>
> Hi Trent,
>
> I didn't see the review of that serie, I'm curious what others said.
> As for my comments, I'll inline your code, sorry about that.
>
> > 02/14: v4l2: Create helper function for bounding and aligning images
> > http://linuxtv.org/hg/~tap/v4l-dvb?cmd=changeset;node=b4d3ec8d363d
> >
> +static unsigned int clamp_align(unsigned int x, unsigned int min,
> + unsigned int max, unsigned int align)
> +{
> + /* Bits that must be zero to be aligned */
> + unsigned int mask = ~((1 << align) - 1);
> +
> + /* Round to nearest aligned value */
> + if (align)
> + x = (x + (1 << (align - 1))) & mask;
>
> If I'm not mistaken, these lines are an equivalent of :
>   balign = 1 << align;
> if (align)
>   x = ALIGN(x + 1 - balign/2, balign);
>
> Isn't that simpler to read ?

I don't think so, it's not obvious that it will round to the nearest value.
You have to look up ALIGN and then __ALIGN_MASK and see that it will in
effect add balign - 1 and then reduce x + 1 - balign/2 + balign - 1 into x
+ balign/2.

It also generates worse code.  You'd think the compiled would be able to
optimize it into the same code, but it doesn't.  Unless there is some issue
with how it will work with negative values that causes a subtle difference.
--
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: [PULL] generic image bounds setting and alignment function

2009-06-01 Thread Robert Jarzmik
Trent Piepho  writes:

> Mauro,
>
> Please pull from http://linuxtv.org/hg/~tap/v4l-dvb
>
> This series adds a function for bounding and alignment image sizes and
> modifies a number of drivers to use it.  It came up when the pxa patches to
> deal with the alignment issues for that driver were posted.  I haven't
> tested these patches for pxa.

Hi Trent,

I didn't see the review of that serie, I'm curious what others said.
As for my comments, I'll inline your code, sorry about that.

> 02/14: v4l2: Create helper function for bounding and aligning images
> http://linuxtv.org/hg/~tap/v4l-dvb?cmd=changeset;node=b4d3ec8d363d
>
+static unsigned int clamp_align(unsigned int x, unsigned int min,
+   unsigned int max, unsigned int align)
+{
+   /* Bits that must be zero to be aligned */
+   unsigned int mask = ~((1 << align) - 1);
+
+   /* Round to nearest aligned value */
+   if (align)
+   x = (x + (1 << (align - 1))) & mask;

If I'm not mistaken, these lines are an equivalent of :
balign = 1 << align;
if (align)
x = ALIGN(x + 1 - balign/2, balign);

Isn't that simpler to read ?

+
+   /* Clamp to aligned value of min and max */
+   if (x < min)
+   x = (min + ~mask) & mask;
+   else if (x > max)
+   x = max & mask;
+
+   return x;
+}

> 03/14: pxa-camera: Use v4l bounding/alignment function
> http://linuxtv.org/hg/~tap/v4l-dvb?cmd=changeset;node=cb48209c1841

+ /* Limit to pxa hardware capabilities. YUV422P planar format requires
+ * images size to be a multiple of 16 bytes. If not, zeros will be
+ * inserted between Y and U planes, and U and V planes, which violates
+ * the YUV422P standard. */
The multiple lines comment format, according to the Coding Style, would be
rather :
/*
 *
 */
That is a question for Guennadi, as he is the maintainer of pxa_camera.

+ v4l2_bound_align_image(&pix->width, 48, 2048, 1,
+ &pix->height, 32, 2048, 0,
+ xlate->host_fmt->fourcc == V4L2_PIX_FMT_YUV422P ? 4 : 0);

Cheers.

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