[Webkit-unassigned] [Bug 167544] [GTK] Do not release OpenGL resource immediately when leaving accelerated compositing mode

[bugzilla-daemon at webkit.org]

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

Michael Catanzaro <mcatanzaro at igalia.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #300011|review?                     |review+
              Flags|                            |

--- Comment #3 from Michael Catanzaro <mcatanzaro at igalia.com> ---
Comment on attachment 300011
  --> https://bugs.webkit.org/attachment.cgi?id=300011
Patch

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

Nice!

Now I know all this recent work to bring back on-demand accelerated composting is going to have a dramatic memory reduction benefit, but these changes seem risky and I don't think we should backport them to 2.14 now that we're nearly to the end of the release cycle. My recommendation is to let it bake in trunk at this point. Not just this commit, but the whole set. It's your call, of course.

> Source/WebKit2/ChangeLog:24
> +        starting a timer of 5 secons to discard it if not reused.

seconds

> Source/WebKit2/WebProcess/WebPage/AcceleratedDrawingArea.cpp:382
> +    m_previousLayerTreeHost = WTFMove(m_layerTreeHost);

If something ever goes wrong, better to have a null pointer dereference than a use after free, so I think you should set m_layerTreeHost to nullptr here, even if it's not strictly necessary. And it looks like it *is* necessary, since you have checks in several places to only use m_previousLayerTreeHost if m_layerTreeHost is null. If it's really not needed, please explain why.

> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/ThreadedCoordinatedLayerTreeHost.cpp:229
> +    m_isDiscardable = discardable;
> +    if (m_isDiscardable) {
> +        m_discardableSyncActions = OptionSet<DiscardableSyncActions>();
> +        return;
> +    }

Personally, I would split the rest of this function below this point into a new function named ThreadedCoordinatedLayerTreeHost::applyDiscardableSyncActions.

> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/ThreadedCoordinatedLayerTreeHost.h:111
> +    enum class DiscardableSyncActions { UpdateSize = 1 << 1, UpdateViewport = 1 << 2, UpdateScale = 1 << 3, UpdateBackground = 1 << 4 };

I would also prefer not to write enums on one line like this.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20170128/81aa4f9b/attachment.html>