Bug 168128

Summary: [GTK] Non-accelerated drawing is broken with HiDPI
Product: WebKit Reporter: Adrian Perez <aperez>
Component: WebKitGTKAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Critical CC: berto, bugs-noreply, cgarcia, jbicha, magomez, mcatanzaro, mike, public, zan
Priority: P2    
Version: Other   
Hardware: Unspecified   
OS: Linux   
Attachments:
Description Flags
xdpyinfo for current laptop (failing, Intel HD Graphics 520)
none
xdpyinfo for old laptop (working, Intel Haswell-ULT Graphics)
none
Patch to add debug-prints in DrawingAreaImpl.cpp
none
Patch mcatanzaro: review+

Description Adrian Perez 2017-02-10 10:25:56 PST
The WebKitGTK+ 2.14.4 release has just hit the Arch Linux “testing” repository.
After installing it, most webpages won't paint anything inside the webview in
Epiphany. The content is loaded, and moving the mouse around makes the URL tooltip
appear in the bottom-left corner, and the mouse cursor changes to a pointing hand,
which means that WebKitGTK+ is doing some work but no updates reach the graphics
output. In particular:

 - webkitgtk.org → blank webview
 - planet.gnome.org → blank webview
 - bugs.webkit.org → blank webview
 - github.com → works

The fact that GitHub worked led me to try the following:

  % WEBKIT_FORCE_COMPOSITING_MODE=1 epiphany    # All websites work
  % WEBKIT_DISABLE_COMPOSITING_MODE=1 epiphany  # No website works

I have confirmed that the same is true for MiniBrowser after building the current
Git “master”.
Comment 1 Adrian Perez 2017-02-10 10:30:45 PST
Also, I tried the following:

 - Resizing the Epiphany/MiniBrowser window does not seem to trigger
   repaints. With AC disabled (either because it does not get enabled
   on-demand, or disabled with the environment variable), the “contents”
   of the blank webview show some flickering.

 - I have also updated to WebKitGTK+ 2.14.4 in my old laptop, also running
   Arch Linux. Painting works normally in that one. The OS setups are about
   the same, the main difference being the hardware. I'll try and post what
   is exactly different, as it seems that could help out identifying the
   root cause of the issue.
Comment 2 Adrian Perez 2017-02-10 10:35:18 PST
Created attachment 301173 [details]
xdpyinfo for current laptop (failing, Intel HD Graphics 520)
Comment 3 Adrian Perez 2017-02-10 10:36:30 PST
Created attachment 301174 [details]
xdpyinfo for old laptop (working, Intel Haswell-ULT Graphics)
Comment 4 Adrian Perez 2017-02-10 10:54:23 PST
The most obvious difference is that in my old laptop I got DRI3 disabled
at some point (using “Option "DRI" "2"” in a xorg.conf.d snippet). Disabling
DRI3 with the environment variable did not solve the issue:

  % LIBGL_DRI3_DISABLE=1 Tools/Scripts/run-minibrowser --gtk --debug  # Still blanks

Editing “/etc/X11/xorg.conf.d/99-intel.conf” and adding the option there
to disable DRI3, and launching a new X session makes WebKitGTK+ paint
webviews correctly without having to set any environment variable. For
reference, these are the contents of that file in my system:

  % cat /etc/X11/xorg.conf.d/99-intel.conf 
  Section "Device"
      Identifier "Intel Graphics"
      Driver     "intel"
      Option     "DRI" "2"   # Added for testing.
      Option     "Backlight" "intel_backlight"
  EndSection

With the configuration snippet above, non-AC mode works.
Comment 5 Michael Catanzaro 2017-02-10 18:57:51 PST
Well crap!
Comment 6 Carlos Garcia Campos 2017-02-10 23:32:32 PST
I'm surprised DRI setting affects the non AC mode.
Comment 7 Tim Ruffing 2017-02-11 10:45:34 PST
I have the same issue on Arch Linux with 2.14.4, pretty annoying because evolution does not render emails.

Setting WEBKIT_FORCE_COMPOSITING_MODE=1 does NOT work for me. 
DRI is set to 3 in Xorg config.
Comment 8 Michael Catanzaro 2017-02-11 12:53:51 PST
(In reply to comment #7)
> Setting WEBKIT_FORCE_COMPOSITING_MODE=1 does NOT work for me. 
> DRI is set to 3 in Xorg config.

Because evolution sets WEBKIT_DISABLE_COMPOSITING_MODE. It should not do that.
Comment 9 Michael Catanzaro 2017-02-11 16:41:15 PST
*** Bug 168180 has been marked as a duplicate of this bug. ***
Comment 10 Adrian Perez 2017-02-12 04:14:50 PST
Created attachment 301303 [details]
Patch to add debug-prints in DrawingAreaImpl.cpp

With this patch applied, running MiniBrowser results in the following output:

  % run-minibrowser --gtk --debug        
  Starting MiniBrowser.
 DrawingAreaImpl: instantiated!
  updatePreferences
    AC enabled: yes
    AC forced: no
    layerTreeHost: 0x7fea80741fdd
  updateBackingStoreState
    m_forceRepaintAfterBackingStoreStateUpdate: no
  enterAcceleratedCompositingMode((nil))
  setNeedsDisplay
    m_layerTreeHost -> AcceleratedDrawingArea::setNeedsDisplay()
  setNeedsDisplayInRect
    m_layerTreeHost -> AcceleratedDrawingArea::setNeedsDisplayInRect()
  ...
  (keeps repeating until resizing the window)
  ...
  mainFrameContentSizeChanged
    m_layerTreeHost -> delegating
  setNeedsDisplayInRect
    m_layerTreeHost -> AcceleratedDrawingArea::setNeedsDisplayInRect()
  ...
  (keeps repeating)

The output with “WEBKIT_DISABLE_COMPOSITING_MODE=1” is the same except for
the 4th line, which report “AC enabled: no”. The, with AC forced, the output
becomes:

  % WEBKIT_FORCE_COMPOSITING_MODE=1 run-minibrowser --gtk --debug
  Starting MiniBrowser.
  DrawingAreaImpl: instantiated!
  updatePreferences
    AC enabled: yes
    AC forced: yes
    layerTreeHost: 0x7f7997961fdd
    will call enterAcceleratedCompositingMode(nullptr)
  enterAcceleratedCompositingMode((nil))
  updateBackingStoreState
    m_forceRepaintAfterBackingStoreStateUpdate: no
  setNeedsDisplay
    m_layerTreeHost -> AcceleratedDrawingArea::setNeedsDisplay()
  setNeedsDisplayInRect
    m_layerTreeHost -> AcceleratedDrawingArea::setNeedsDisplayInRect()
  setNeedsDisplayInRect
    m_layerTreeHost -> AcceleratedDrawingArea::setNeedsDisplayInRect()
  ...
  (repeats a few times)
  ...
  mainFrameContentSizeChanged
    m_layerTreeHost -> delegating
  setNeedsDisplayInRect
    m_layerTreeHost -> AcceleratedDrawingArea::setNeedsDisplayInRect()
  setRootCompositingLayer
  setNeedsDisplayInRect
    m_layerTreeHost -> AcceleratedDrawingArea::setNeedsDisplayInRect()
  setNeedsDisplayInRect
    m_layerTreeHost -> AcceleratedDrawingArea::setNeedsDisplayInRect()
  didUpdateBackingStoreState
  setNeedsDisplay
    m_layerTreeHost -> AcceleratedDrawingArea::setNeedsDisplay()
  sendDidUpdateBackingStoreState
  setRootCompositingLayer
  setRootCompositingLayer
  setNeedsDisplayInRect
    m_layerTreeHost -> AcceleratedDrawingArea::setNeedsDisplayInRect()
  setNeedsDisplayInRect
    m_layerTreeHost -> AcceleratedDrawingArea::setNeedsDisplayInRect()
  setNeedsDisplayInRect
    m_layerTreeHost -> AcceleratedDrawingArea::setNeedsDisplayInRect()
  mainFrameContentSizeChanged
    m_layerTreeHost -> delegating
  mainFrameContentSizeChanged
    m_layerTreeHost -> delegating

With AC disabled, methods in AcceleratedDrawingArea are also called, which sounds
odd in my head (note that I am not familiar with the AC vs. non-AC code paths, I am
just pointing out what sounds odd to me). What should be the condition that determines
the code path to be followed? Currently there are are many checks for a valid pointer
DrawingAreaImpl::m_layerTreeHost, which seems to be one the main decision point to
decide whether to forward calls to methods in AcceleratedDrawingArea or not, but it
seems that ::m_layerTreeHost is always a valid pointer despite the values used for
the environment variables... Is that correct? If it's going to be always valid, why
all the checks?
Comment 11 Adrian Perez 2017-02-12 04:18:39 PST
(In reply to comment #9)
> *** Bug 168180 has been marked as a duplicate of this bug. ***

Ah, good catch, I forgot to mention that I have HiDPI here as well. So the
issue seems to be manifest itself in HiDPI+DRI3.
Comment 12 Adrian Perez 2017-02-12 04:22:58 PST
For comparison, output with HiDPI disables (scale-factor==1): 

  % WEBKIT_DISABLE_COMPOSITING_MODE=1 run-minibrowser --gtk --debug
  Starting MiniBrowser.
  DrawingAreaImpl: instantiated!
  updatePreferences
    AC enabled: no
    AC forced: no
    layerTreeHost: 0x7fce46674fdd
  updateBackingStoreState
    m_forceRepaintAfterBackingStoreStateUpdate: no
  setNeedsDisplay
  setNeedsDisplayInRect
    continuing -> scheduleDisplay()
  scheduleDisplay
  setNeedsDisplayInRect
    continuing -> scheduleDisplay()
  ...
  (repeats a few times)
  ...
  scheduleDisplay
  setNeedsDisplayInRect
    dirty.isEmpty() -> early return
  mainFrameContentSizeChanged
  setNeedsDisplayInRect
    continuing -> scheduleDisplay()
  scheduleDisplay
  didUpdateBackingStoreState
  setNeedsDisplay
  setNeedsDisplayInRect
    continuing -> scheduleDisplay()
  scheduleDisplay
  sendDidUpdateBackingStoreState
  display
  shouldPaintBoundsRect
  setNeedsDisplayInRect
    continuing -> scheduleDisplay()
  scheduleDisplay
  setNeedsDisplayInRect
    continuing -> scheduleDisplay()
  scheduleDisplay
  setNeedsDisplayInRect
    continuing -> scheduleDisplay()
  scheduleDisplay
  displayTimerFired
  display
  display
  shouldPaintBoundsRect
  didUpdate
  displayTimerFired
  display
  display: empty dirty region -> early return
  setNeedsDisplayInRect
    continuing -> scheduleDisplay()
  scheduleDisplay
  setNeedsDisplayInRect
    continuing -> scheduleDisplay()
  ...
Comment 13 Adrian Perez 2017-02-12 04:28:27 PST
Note how, with HiDPI disabled, m_layerTreeHost==nullptr when control
flow reaches DrawingAreaImpl::mainFrameContentSizeChanged() because
the print-debug “m_layerTreeHost -> delegating” is not logged. I wonder
what causes m_layerTreeHost to be a valid pointer with HiDPI+DRI3, but
it certainly seems to be the crux of this issue.
Comment 14 Tim Ruffing 2017-02-12 06:04:09 PST
Just to confirm the relevance... I have also HiDPI here.
Comment 15 Carlos Garcia Campos 2017-02-12 23:25:42 PST
This has nothing to do with DRI3, the only problem is the device scale factor.
Comment 16 Carlos Garcia Campos 2017-02-12 23:34:32 PST
Created attachment 301335 [details]
Patch
Comment 17 Adrian Perez 2017-02-13 01:29:43 PST
(In reply to comment #15)
> This has nothing to do with DRI3, the only problem is the device scale
> factor.

Right, I forgot to mention that my old laptop, apart from having DRI3
disabled, does _not_ have a HiDPI screen. 

I built WebKitGTK+ locally with your patch applied, and I can confirm
that MiniBrowser correctly paints websites now, both using 1x and 2x
scale-factor. Tested with all the combinations of environment variables:

  % run-minibrowser --gtk --debug   # No environment varible.
  % WEBKIT_DISABLE_COMPOSITING_MODE=1 run-minibrowser --gtk --debug
  % WEBKIT_FORCE_COMPOSITING_MODE=1 run-minibrowser --gtk --debug

So informal r+ from me — now that I see what it does, it makes sense.

                                - - - - -

Side question: is there some way in which we could avoid this kind of
regression in the future? It would be interesting to do some automated
smoketesting, like building and loading a few selected websites with
MiniBrowser under the control of a script, capturing the screen, and
checking that there are non-blank pixels on a capture of the window, for
different scale-factors and AC/non-AC modes.
Comment 18 Carlos Garcia Campos 2017-02-13 01:55:39 PST
(In reply to comment #17)
> (In reply to comment #15)
> > This has nothing to do with DRI3, the only problem is the device scale
> > factor.
> 
> Right, I forgot to mention that my old laptop, apart from having DRI3
> disabled, does _not_ have a HiDPI screen. 
> 
> I built WebKitGTK+ locally with your patch applied, and I can confirm
> that MiniBrowser correctly paints websites now, both using 1x and 2x
> scale-factor. Tested with all the combinations of environment variables:
> 
>   % run-minibrowser --gtk --debug   # No environment varible.
>   % WEBKIT_DISABLE_COMPOSITING_MODE=1 run-minibrowser --gtk --debug
>   % WEBKIT_FORCE_COMPOSITING_MODE=1 run-minibrowser --gtk --debug
> 
> So informal r+ from me — now that I see what it does, it makes sense.
> 
>                                 - - - - -
> 
> Side question: is there some way in which we could avoid this kind of
> regression in the future? It would be interesting to do some automated
> smoketesting, like building and loading a few selected websites with
> MiniBrowser under the control of a script, capturing the screen, and
> checking that there are non-blank pixels on a capture of the window, for
> different scale-factors and AC/non-AC modes.

We should enable hidpi layout tests.
Comment 19 Carlos Garcia Campos 2017-02-13 05:43:53 PST
Committed r212228: <http://trac.webkit.org/changeset/212228>