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