[Webkit-unassigned] [Bug 43903] Speeding up SVG filters with multicore (SMP) support

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=43903





--- Comment #3 from Nikolas Zimmermann <zimmermann at kde.org>  2010-11-30 05:48:36 PST ---
(From update of attachment 75125)
View in context: https://bugs.webkit.org/attachment.cgi?id=75125&action=review

The idea looks great, the video you showed is also very convincing! :-)
Some early review comments:

> WebCore/platform/graphics/filters/FELighting.cpp:240
> +            data.interior(offset, normalVector);

Hm, not related to this patch, but 'interior' is a bad name, what does it do?

> WebCore/platform/graphics/filters/FELighting.cpp:246
> +#ifdef FILTER_THREAD

This should use the USE(FILTER_THREADS) or ENABLE(FILTER_THREADS) logic. (not sure which one to use, need to read Maciejs mail again regarding that topic, when to use USE or ENABLE, but I guess USE is approriate here).

> WebCore/platform/graphics/filters/FELighting.cpp:249
> +    PaintThreadData& data = *reinterpret_cast<PaintThreadData*>(parameter);

How about adding ASSERT(parameter) as first argument.

> WebCore/platform/graphics/filters/FELighting.cpp:280
> +    if (width >= 3 && height >= 10 && width * height > 4000) {

I'd like a comment here, that this is emperical data.

> WebCore/platform/graphics/filters/FELighting.h:79
> +        {

An ASSERT(filter) can't hurt.
It looks awkward to use filter(filter) and lightingData(lightingData) - is that guarenteed to work? I was not aware that this is possible!

> WebCore/platform/graphics/filters/FETurbulence.cpp:335
> +#ifdef FILTER_THREAD

Use USE(FILTER_THREADS).

> WebCore/platform/graphics/filters/FETurbulence.cpp:338
> +    PaintThreadData& data = *reinterpret_cast<PaintThreadData*>(parameter);

Maybe ASSERT(parameter) first.

> WebCore/platform/graphics/filters/FETurbulence.cpp:360
> +#ifdef FILTER_THREAD

Same comment, use USE(FILTER_THREADS), won't repeat it for the other occourences.

> WebCore/platform/graphics/filters/FETurbulence.cpp:362
> +    if (imageRect.height() >= 6 && imageRect.width() * imageRect.height() > 4000) {

Emperical data comment...

> WebCore/platform/graphics/filters/FETurbulence.h:95
> +        {

ASSERT(filter)

> WebCore/platform/graphics/filters/FilterEffect.cpp:33
> +#ifdef FILTER_THREAD
> +ThreadIdentifier FilterEffect::s_threadID;
> +FilterEffect::ThreadShared* FilterEffect::s_threadShared;
> +bool FilterEffect::s_stopThread;
> +#endif

We generally disallow globals, so how about putting these in functions?
static ThreadIdentifier threadIdentifier()
{
    static ThreadIdentifier s_threadID;
    return s_threadID;
}

Are you avoiding the use of DEFINE_STATIC_LOCAL on purpose?
I have never used threads in WebCore so far, forgive my ignorance if I'm overlooking something obvious.

> WebCore/platform/graphics/filters/FilterEffect.cpp:141
> +        s_threadShared->m_cond.wait(s_threadShared->m_mutex);

Is this prefereedd style when using threads? Are shall we expose accesors for m_cond, m_start, m_finish, m_function, m_mutex etc.?

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.