HSV manipulation operator for GPU & CPU #1338
Conversation
Signed-off-by: Michał Szołucha <mszolucha@nvidia.com>
Signed-off-by: Michał Szołucha <mszolucha@nvidia.com>
Signed-off-by: Michał Szołucha <mszolucha@nvidia.com>
Signed-off-by: Michał Szołucha <mszolucha@nvidia.com>
Signed-off-by: Michał Szołucha <mszolucha@nvidia.com>
Signed-off-by: Michał Szołucha <mszolucha@nvidia.com>
Signed-off-by: Michał Szołucha <mszolucha@nvidia.com>
Signed-off-by: Michał Szołucha <mszolucha@nvidia.com>
Signed-off-by: Michał Szołucha <mszolucha@nvidia.com>
dali/pipeline/operators/color/hsv.h
Outdated
| }}; | ||
|
|
||
|
|
||
| inline mat3 compose_hue(float hue /* hue hue hue */ ) { |
dali/pipeline/operators/color/hsv.h
Outdated
| ret.resize(size); | ||
| for (size_t i = 0; i < size; i++) { | ||
| ret[i] = Yiq2Rgb * compose_hue(hue[i]) * compose_saturation(saturation[i]) * | ||
| compose_value(value[i]) * Rgb2Yiq; |
There was a problem hiding this comment.
Why do you insist on having these matrix multiplications here? Especially the perfectly avoidable compose_value (you'd be better off just multiplying by the scalar). And, as I demonstrated before, the function that does all three transforms is not that complicated, either.
There was a problem hiding this comment.
I think that this is ok from readability standpoint. You define those transformations as rotations/scaling/shifting and compose them into one transformation.
There was a problem hiding this comment.
When we're at readability: how about:
mat3 color_transform = rotate_hue(hue[i]) * scale_saturation(saturation[i]) * scale_value(value[i]);
return Yiq2Rgb * color_transform * Rgb2Yiq;
These compose_xxx names don't really tell us what we do with xxx.
There was a problem hiding this comment.
Well, rotate_hue isn't the best name either, cause it doesn't actually rotate anything.
How about compose_hue_matrix? Or sth shorter:
/**
* Composes transformation matrix for hue
*/
mat3 hue_mat(){}
[...]
return Yiq2Rgb * hue_mat(hue[i]) * sat_mat(saturation[i]) * val_mat(value[i]) * Rgb2Yiq;
?
dali/pipeline/operators/color/hsv.h
Outdated
|
|
||
|
|
||
| template <typename Backend> | ||
| class Hsv : public Operator<Backend> { |
There was a problem hiding this comment.
Please rename to HsvOp or something along that line - Hsv is a good name for a structure that contains uint8_t h, s, v or a simple conversion function, but doesn't quite cut it for a full-blown operator.
dali/pipeline/operators/color/hsv.h
Outdated
| explicit Hsv(const OpSpec &spec) : | ||
| Operator<Backend>(spec), | ||
| output_type_(spec.GetArgument<DALIDataType>(hsv::kOutputType)) { | ||
| GetSingleOrRepeatedArg(spec, hue_, hsv::kHue, batch_size_); |
There was a problem hiding this comment.
This is bad. The arguments are not meant to be vectors, they should be either scalars or tensors.
|
!build |
|
CI MESSAGE: [940207]: BUILD STARTED |
|
CI MESSAGE: [940207]: BUILD FAILED |
|
CI MESSAGE: [940717]: BUILD STARTED |
Signed-off-by: Michał Szołucha <mszolucha@nvidia.com>
|
!build |
|
CI MESSAGE: [940845]: BUILD STARTED |
|
CI MESSAGE: [940845]: BUILD FAILED |
|
!build |
|
CI MESSAGE: [940953]: BUILD STARTED |
|
CI MESSAGE: [940717]: BUILD PASSED |
|
CI MESSAGE: [940953]: BUILD PASSED |
Why we need this PR?
Pick one