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
