Closed GoogleCodeExporter closed 9 years ago
Original comment by andrew.k...@gmail.com
on 2 Feb 2012 at 9:50
Original comment by andrew.k...@gmail.com
on 2 Feb 2012 at 8:55
1) Documentation, like the one shown below sounds quite funny and confusing. To
me it sounds like: "Not sure what is the point of this parameter, but you can
play with it." If you know what the property is for, why not to explain it in
the code docs?
"/// You may play with it if you'd like to but in most cases default values
should be fine"
2) Tweaking the algorithm for 24bpp (as you wrote in comments) does not have
much sense. If you did not hard code pixel size to 3, but checked it from the
provided image, the filter would easily work with pixel formats like 32 bit
RGB. Also it can work for 32 bit ARGB, if we do smoothing of RGB channels only
and ignore Alpha value. Also 8bpp grayscale version does not seem to be too
difficult to implement.
Original comment by andrew.k...@gmail.com
on 3 Feb 2012 at 9:59
Good remarks.
1. I've got a mathcad file which shows a func for Power factors, it's not thet
easy to describe... But I'll try to find some other words.
2. Hardcoding pixel size let me avoid extra loops inside the current loop
(which is already 4 level nested). I'm quite concerned about the performance of
the filter since it's quite bulky in it's core algorithm. Due to little tricks
(except parallel processing) I've improved the original implementation (with
single method, multiple pixel formats, functions evaluated in the loop body
etc.) to smth about 2x-2.5x.
If from your experience you consider that having native support (without
transformations) of 8bpp and 32bpp formats (any other) I may suggest not the
best OOD but good from performance point of view aproach with presenting a
number of Process methods which will work with specific pixel formats.
Original comment by smaxm...@gmail.com
on 4 Feb 2012 at 7:04
In the attachment you may find an updated .cs file.
I've amended XML summaries for XxxPower properties and also added support for
8bpp and 32 bpp formats.
The file increased in size significantly due to much repeating code which is
necessary to get some performance benefits. I used the same approach and was
targeting to have as few calls/operations in loop bodies as possible.
What do you think?
Original comment by smaxm...@gmail.com
on 6 Feb 2012 at 2:36
Attachments:
[deleted comment]
Is it a bug?
spatialFunc[i, k] = M.Exp(-0.5 * (M.Pow(M.Sqrt((i - c) * (i - c) + (k - c) * (k
- c) / spatialFactor), SpatialPower)));
>> (i - c) * (i - c) + (k - c) * (k - c) / spatialFactor
Should it be ???
( (i - c) * (i - c) + (k - c) * (k - c) ) / spatialFactor
Original comment by andrew.k...@gmail.com
on 6 Feb 2012 at 8:42
You're right, brackets were missing. The updated file is attached.
P.S.: Another code review pro.
Original comment by smaxm...@gmail.com
on 7 Feb 2012 at 8:00
Attachments:
I would say the documentation is still confusing ... Did not find any
difference in documentation for SpatialPower and ColorFactor - it is identical.
I can see these value are used to initialize different table, so I would expect
they should have slightly different effect.
Original comment by andrew.k...@gmail.com
on 8 Feb 2012 at 8:32
The code is merged in development trunk after code review.
Updating Bilateral filter implementation after code review:
1) Did some polishing of the code and documentation so it looks more consistent
to the rest of the framework;
2) Removed confusing documentation like “play with it and discover
yourself”;
3) Fixed implementation of partial filtering, when a filter can be applied to
rectangle of an image (original implementation did not care much about it);
4) Did some minor improvements, which result in slightly better performance
(tests need to be done in Release build running a filter 100+ times to see any
difference).
TODO: Documentation for filter properties still might be updated to clarify
things.
Committed in revisions 1668-1669 (2 commits so result of code review could be
seen as diff). Will be released in version 2.2.4.
P.S. If there are any further updates to the code, please, provide patch to the
version in trunk.
Original comment by andrew.k...@gmail.com
on 8 Feb 2012 at 9:44
Original comment by andrew.k...@gmail.com
on 23 Feb 2012 at 9:14
Original issue reported on code.google.com by
smaxm...@gmail.com
on 29 Jan 2012 at 10:52Attachments: