https://codereview.appspot.com/6603053/diff/1/source/blender/compositor/intern/COM_SocketReader.h File source/blender/compositor/intern/COM_SocketReader.h (right): https://codereview.appspot.com/6603053/diff/1/source/blender/compositor/intern/COM_SocketReader.h#newcode36 source/blender/compositor/intern/COM_SocketReader.h:36: COM_PS_CUBICBSPLINEPREFILTER = 3 Would this actually ever be used ...
11 years, 6 months ago
(2012-10-04 18:45:55 UTC)
#2
https://codereview.appspot.com/6603053/diff/1/source/blender/compositor/inter...
File source/blender/compositor/intern/COM_SocketReader.h (right):
https://codereview.appspot.com/6603053/diff/1/source/blender/compositor/inter...
source/blender/compositor/intern/COM_SocketReader.h:36:
COM_PS_CUBICBSPLINEPREFILTER = 3
Would this actually ever be used anywhere? From looking at the other sampling
modes it seems that nodes would have to explicitly override the sampler
parameter to make use of it.
IIRC there was an experimental node to force a particular sampling mode on
inputs, but it was not intended for production work and only used for testing.
Furthermore you also need to ensure that all nodes which refer to the sampler
input can handle this additional case or fall back to one of the other modes
safely. In particular the MovieClipOperation (which should probably ignore it).
Also ReadBufferOperation would use cubic in this case.
Alternatively this could be made an option just for the image node to override
the requested sampling mode locally for just this node, or replace BICUBIC with
the prefilter method if the option is enabled. See also the next comment!
https://codereview.appspot.com/6603053/diff/1/source/blender/compositor/opera...
File source/blender/compositor/operations/COM_ImageOperation.cpp (right):
https://codereview.appspot.com/6603053/diff/1/source/blender/compositor/opera...
source/blender/compositor/operations/COM_ImageOperation.cpp:149: lockMutex();
Don't think such mutex locks + buffer check should be done in the inner pixel
loop. Of course, to avoid this one would have to know the sampling mode in
advance. Referring to my previous comment, the easiest way to do this would be a
node option "use bspline prefilter" or so, which, if enabled, would always
generate the secondary buffer in initExecution and possibly ignore the requested
sampling mode altogether.
https://codereview.appspot.com/6603053/diff/1/source/blender/compositor/opera...
source/blender/compositor/operations/COM_ImageOperation.cpp:179: lockMutex();
same as previous comment
Thanks so much for taking time to review this Lukas. https://codereview.appspot.com/6603053/diff/1/source/blender/compositor/intern/COM_SocketReader.h File source/blender/compositor/intern/COM_SocketReader.h (right): https://codereview.appspot.com/6603053/diff/1/source/blender/compositor/intern/COM_SocketReader.h#newcode36 ...
11 years, 6 months ago
(2012-10-04 19:07:36 UTC)
#3
Thanks so much for taking time to review this Lukas.
https://codereview.appspot.com/6603053/diff/1/source/blender/compositor/inter...
File source/blender/compositor/intern/COM_SocketReader.h (right):
https://codereview.appspot.com/6603053/diff/1/source/blender/compositor/inter...
source/blender/compositor/intern/COM_SocketReader.h:36:
COM_PS_CUBICBSPLINEPREFILTER = 3
On 2012/10/04 18:45:55, lukas.toenne1 wrote:
> Would this actually ever be used anywhere? From looking at the other sampling
> modes it seems that nodes would have to explicitly override the sampler
> parameter to make use of it.
Indeed. This was a welcome addition to be able to control the scaling algorithm,
so I merely adapted to the existing format.
>
> IIRC there was an experimental node to force a particular sampling mode on
> inputs, but it was not intended for production work and only used for testing.
Bad idea in my mind. See other comment in other file.
>
> Furthermore you also need to ensure that all nodes which refer to the sampler
> input can handle this additional case or fall back to one of the other modes
> safely. In particular the MovieClipOperation (which should probably ignore
it).
I believe this is plug and play useful immediately and would provide a valuable
option to interested artists.
I don't believe MovieClip is an exception here.
> Also ReadBufferOperation would use cubic in this case.
Explain?
>
> Alternatively this could be made an option just for the image node to override
> the requested sampling mode locally for just this node, or replace BICUBIC
with
> the prefilter method if the option is enabled. See also the next comment!
This is certainly increases a cubic accuracy, and in many instances where cubic
is a default, it should be upgraded to this. However, within an artistic choice
role, the option to select approaches is extremely valuable to an artist.
https://codereview.appspot.com/6603053/diff/1/source/blender/compositor/opera...
File source/blender/compositor/operations/COM_ImageOperation.cpp (right):
https://codereview.appspot.com/6603053/diff/1/source/blender/compositor/opera...
source/blender/compositor/operations/COM_ImageOperation.cpp:149: lockMutex();
On 2012/10/04 18:45:55, lukas.toenne1 wrote:
> Don't think such mutex locks + buffer check should be done in the inner pixel
> loop. Of course, to avoid this one would have to know the sampling mode in
> advance. Referring to my previous comment, the easiest way to do this would be
a
> node option "use bspline prefilter" or so, which, if enabled, would always
> generate the secondary buffer in initExecution and possibly ignore the
requested
> sampling mode altogether.
I believe offering sampling method is mandatory. For example, a pixelated effect
zoom would require nearest. So I don't believe removing the selection is a wise
path.
I agree that the per pixel location is awkward, but it would seem it is hard
coded into an architecture that didn't foresee frequency domain filters on
imaging?
Perhaps we can address this with a similar BaseImageOperation mode akin to
setting Comolex mode?
Issue 6603053: CUBIC B SPLINE WITH PREFILTER: Scaling algorithm implementation
Created 11 years, 6 months ago by troy_s
Modified 11 years, 6 months ago
Reviewers: lukas.toenne1
Base URL: https://svn.blender.org/svnroot/bf-blender/trunk/blender/
Comments: 5