Add error checking into OpenCL version of SVG filters.

[Tamas Czene]

If there is an error at OpenCL version of SVG filters, then remove all results and go to the software path.

[Tamas Czene]

Created attachment 183774 [details]
proposed patch

[Stephen Chenney]

I am extremely unhappy about the sudden profusion of #ifdef OPENCL throughout the platform/graphics/filters/FilterEffect code.

The code required to manage OpenCLHandle, and the use of OpenCL as a backing for images, should all be placed in a platform specific version of ImageBuffer. That will get rid of almost all of the code cluttering FilterEffect, and is in fact the correct way to provide a platform specific back end.

Skia manages to provide hardware support for filters with only 2 methods inside a #if USE(SKIA). CG does it too. There is no obvious reason why you cannot do the same.

With specific regard to this patch, code to fallback to software from a hardware rendering would be useful for all platforms. This patch does not supply such a general approach.

[Zoltan Herczeg]

The problem with OpenCL is that it is an extension over the software code path, not a replacement, like the others. The same thing is true for the ImageBuffer, there is only one type allowed for the whole WebKit, so there is no obvious waz to extend it (for me at least). But we would be happy to get rid of these ifdefs if we could find a better a solution (although this patch does not aim for complete refactoring). Please elaborate more about your idea, maybe that could help to cleverly extend the current solution.

[Tamas Czene]

Created attachment 184446 [details]
proposed patch

[Stephen Chenney]

(In reply to comment #3)
> The problem with OpenCL is that it is an extension over the software code path, not a replacement, like the others. The same thing is true for the ImageBuffer, there is only one type allowed for the whole WebKit, so there is no obvious waz to extend it (for me at least). But we would be happy to get rid of these ifdefs if we could find a better a solution (although this patch does not aim for complete refactoring). Please elaborate more about your idea, maybe that could help to cleverly extend the current solution.

I interpret this to mean that you want a special form of image buffer for this case (some filter ops) but you do not want all ImageBuffer operations to go through the new code path. You would still like to have the existing software image buffer implementation.

Starting with that premise, I'll spend some time figuring out a more elegant solution. I'm sure there is one, even if it means we add "FilterImageBuffer" or some such thing.

For Chromium hardware filters we are likely to need a somewhat different but related set of changes. However, the level of abstraction will be different for OpenCL and Skia so it is not clear that a common framework will be possible.

My unhappiness with the #ifdefs should not prevent this change from landing. I will create a separate bug.

[Zoltan Herczeg]

> My unhappiness with the #ifdefs should not prevent this change from landing. > I will create a separate bug.

Thank you.

> I interpret this to mean that you want a special form of image buffer for
> this case (some filter ops) but you do not want all ImageBuffer operations to
> go through the new code path. You would still like to have the existing software image buffer implementation.

Exactly. I would not touch the existing ImageBuffer implementation, since it is full of platform specific code. The OpenCL code path is only for filters (at the moment). When OpenCL is enabled and working, it replaces the entire buffer chain with its own buffer type. If any error occures, it fallback to the software code path, and never tries OpenCL again (should not happen in practice).

[Zoltan Herczeg]

Comment on attachment 184446 [details]
proposed patch

Nice patch, but some minor fixes are needed:

View in context: https://bugs.webkit.org/attachment.cgi?id=184446&action=review

> Source/WebCore/ChangeLog:3
> +        Add error checking into OpenCL version of SVG filters.

Could you add a little more description what this patch does?

> Source/WebCore/platform/graphics/filters/FilterEffect.cpp:484
> +    int errorCode;

Is this error code set to 0 if an error occures?

> Source/WebCore/platform/graphics/filters/FilterEffect.cpp:515
> +        if (context->inError())
> +            return;
>          context->openCLTransformColorSpace(m_openCLImageResult, absolutePaintRect(), m_resultColorSpace, dstColorSpace);

I think this error check should be done by openCLTransformColorSpace.

> Source/WebCore/platform/graphics/gpu/opencl/FilterContextOpenCL.cpp:96
> +        deleteResource(m_matrixOperation);

freeResource?

> Source/WebCore/platform/graphics/gpu/opencl/FilterContextOpenCL.cpp:112
> +        deleteResource(m_transformColorSpaceKernel);
> +        deleteResource(m_transformColorSpaceProgram);
> +

Extra newline

> Source/WebCore/platform/graphics/gpu/opencl/FilterContextOpenCL.cpp:183
> +    m_transformColorSpaceCompileStatus = openclCompileSuccessful;

What about partially completed compilations? I think the 3 states of these status flags are not necessary anymore after we introduced the error status of the context. They should be simply bools, and tell whether the compilation was attempted.

> Source/WebCore/platform/graphics/gpu/opencl/FilterContextOpenCL.h:173
> +    static void deleteResource(cl_kernel);
> +    static void deleteResource(cl_program);

Shouldn't these accept references? (cl_kernel& and cl_program&) Otherwise the = 0 has no effect. Furthermore these fuctions should be inline.

> Source/WebCore/platform/graphics/gpu/opencl/OpenCLFETurbulence.cpp:176
> +    if (inError())
> +        return;
> +
>      if (m_turbulenceCompileStatus != openclNotCompiledYet)
> -        return m_turbulenceCompileStatus == openclCompileSuccessful;
> +        return;

These two can be combined together, and likely the first one is not needed at all.

[Tamas Czene]

Created attachment 186614 [details]
proposed patch

[Tamas Czene]

Created attachment 186615 [details]
proposed patch

[Zoltan Herczeg]

Comment on attachment 186615 [details]
proposed patch

I think we are getting closer!

View in context: https://bugs.webkit.org/attachment.cgi?id=186615&action=review

> Source/WebCore/platform/graphics/filters/FilterEffect.cpp:489
> +        &clImageFormat, m_absolutePaintRect.width(), m_absolutePaintRect.height(), 0, source, &errorCode);
> +
> +    if (errorCode) {

This newline is unnecessary.

> Source/WebCore/platform/graphics/gpu/opencl/FilterContextOpenCL.cpp:163
> +    if (inError() || m_transformColorSpaceCompileStatus != openclNotCompiledYet)
> +        return;

This transformColorSpaceCompileStatus should be a boolean. Name: transformColorSpaceWasCompiled. And the value should be set to true right after this check. The inError() can be checked after after that (this is faster since only one condition needs to be tested in most cases). Or at least swap the two sides of the || operation.

> Source/WebCore/platform/graphics/gpu/opencl/FilterContextOpenCL.cpp:177
> +    m_transformColorSpaceCompileStatus = openclCompileSuccessful;

Setting this value case is too late here. See above.

> Source/WebCore/platform/graphics/gpu/opencl/FilterContextOpenCL.h:141
> +                if (m_error)
> +                    return 0;
>                  return handle;

I think this could be shortened to return !m_error ? handle : 0;

> Source/WebCore/platform/graphics/gpu/opencl/OpenCLFEColorMatrix.cpp:78
>      m_colorMatrixProgram = compileProgram(colorMatrixKernelProgram);
> -    if (!m_colorMatrixProgram)
> -        return false;
> +    if (!m_colorMatrixProgram)  {
> +        setInError();
> +        return;
> +    }

There are many such tests in the code. They could be shortened as:
if (!(m_colorMatrixProgram = compileProgram(colorMatrixKernelProgram))) {
    setInError();
    return;
}

Or perhaps I have an even better idea. The setInError() calls could be replaced by an inline function, called checkError

bool isFailed(bool value) {
    if (!value)
        setInError();
}

if (isFailed((m_colorMatrixProgram = compileProgram(colorMatrixKernelProgram))))
    return;

We might add various isFailed methods if gcc expect them.

[Zoltan Herczeg]

> bool isFailed(bool value) {
>     if (!value)
>         setInError();
      return !value;
> }

[Tamas Czene]

Created attachment 187120 [details]
proposed patch

[Tamas Czene]

Created attachment 187589 [details]
proposed patch

[WebKit Review Bot]

Attachment 187589 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/platform/graphics/filters/FilterEffect.cpp', u'Source/WebCore/platform/graphics/filters/FilterEffect.h', u'Source/WebCore/platform/graphics/gpu/opencl/FilterContextOpenCL.cpp', u'Source/WebCore/platform/graphics/gpu/opencl/FilterContextOpenCL.h', u'Source/WebCore/platform/graphics/gpu/opencl/OpenCLFEColorMatrix.cpp', u'Source/WebCore/platform/graphics/gpu/opencl/OpenCLFETurbulence.cpp', u'Source/WebCore/rendering/svg/RenderSVGResourceFilter.cpp']" exit_code: 1
Source/WebCore/platform/graphics/gpu/opencl/OpenCLFETurbulence.cpp:230:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 1 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Tamas Czene]

Created attachment 187595 [details]
proposed patch

[Zoltan Herczeg]

Comment on attachment 187595 [details]
proposed patch

Now we are quite close! Only a few more comments:

View in context: https://bugs.webkit.org/attachment.cgi?id=187595&action=review

> Source/WebCore/platform/graphics/filters/FilterEffect.cpp:284
> +    int errorCode = 0;

Do we really need this variable?

> Source/WebCore/platform/graphics/filters/FilterEffect.cpp:293
> +    errorCode = clFinish(context->commandQueue());
> +    if (context->isFailed(errorCode))
> +        return 0;

This could be combined into one check, without the errorCode.

> Source/WebCore/platform/graphics/gpu/opencl/FilterContextOpenCL.cpp:163
> +    if (m_transformColorSpaceWasCompiled)
> +        return true;

This is incorrect. m_transformColorSpaceWasCompiled can be true, even if there is an error.

> Source/WebCore/platform/graphics/gpu/opencl/FilterContextOpenCL.cpp:166
> +    if (inError())
> +        return false;

I think this is the correct form:

if (m_transformColorSpaceWasCompiled || inError())
    return !inError();

> Source/WebCore/platform/graphics/gpu/opencl/FilterContextOpenCL.cpp:240
>      errorNumber = clBuildProgram(program, 0, 0, 0, 0, 0);

Same as above. This variable seems unnecessary.

> Source/WebCore/platform/graphics/gpu/opencl/FilterContextOpenCL.h:75
> +    inline bool resourceAllocationIsFailed(bool);

I am thinking about this name for some time. Perhaps isResourceAllocationFailed a better name...

[Tamas Czene]

Created attachment 187802 [details]
proposed patch

[Zoltan Herczeg]

Comment on attachment 187802 [details]
proposed patch

r=me

[WebKit Review Bot]

Comment on attachment 187802 [details]
proposed patch

Clearing flags on attachment: 187802

Committed r142596: <http://trac.webkit.org/changeset/142596>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.