1319554 - Some colors are suddenly way off in images and text after upgrade to FF50

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Lee McFarland]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:49.0) Gecko/20100101 Firefox/50.0
Build ID: 20160916101415

Steps to reproduce:

On my XUbuntu 16.04 (XFCE GUI) that just automatically upgraded FF50: 
1. Brought up my personally written and VERY simple home page of links normal color blue - came up blue
2. Brought up usatoday.com home page - pictures are normal
3. 'Google searched' and brought up a color wheel image - was normal
-------
4. Came into the system from another computer  using remote desktop connection (xrdp)
5. Brought up my personally written and VERY simple home page of links normal color blue - came up red.  Tried adding html code to change color, some colors such as green come up green but blue will not.  
2. Brought up usatoday.com home page - pictures are weird - not totally negative but definately shifted
3. 'Google searched' and brought up a color wheel image - was abnormal.  The various colors are present however, they are shifted ie. what was blue is something else. In the google.com, a few items are in blue.  Most colors are different.
----------------------
Regressed to a copy of FF 49 - all worked as expected.  


Actual results:

steps include observations of what happened


Expected results:

Expectation was no color difference for remote connection.

Comment 1

[Loic]

Could you narrow down a regresison range with the tool Mozregression (you need python 2.7).
See http://mozilla.github.io/mozregression/ for details.

Run the command "mozregression --good=4ç (or 48)" then copy here the final pushlog from the repository mozilla-inbound.

Comment 2

[Loic]

--good=49, ofc. :)

Comment 3

[Lee McFarland]

I used what appears to be mozregression through several iterations.  By the way, prior to this I had never heard of mozregression. 
It seems to hang or somehow stop when it gives an indication it has only one or three iterations left.  Please advise if I should run
it again and, what parameters would cause it to quickly go to the final iteration.  I've saved about 535 lines from my terminal session.
It looks like a build of 7/15 is good and a build of 7/17 is bad.  The below lines may be even more specific.
Below are what looks like the last few iterations.  
146:23.46 INFO: Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=1983612a169bfc1720ef48b9371e65bd0ec9eef1&tochange=586815d86b50e44a1cc48c734c4da6efcd8ae8ff

146:23.46 INFO: Using local file: /tmp/tmpw3Sq5U/59d49cef6561--mozilla-inbound--target.tar.bz2 (downloaded in background)
146:23.46 INFO: Running mozilla-inbound build built on 2016-07-14 13:08:50.903000, revision 59d49cef
146:36.32 INFO: Launching /tmp/tmpxoQCH2/firefox/firefox
146:36.32 INFO: Application command: /tmp/tmpxoQCH2/firefox/firefox -profile /tmp/tmpSgoIu8.mozrunner
146:36.36 INFO: application_buildid: 20160714120846
146:36.36 INFO: application_changeset: 59d49cef6561960e4169daa19f901a24c57abdd5
146:36.36 INFO: application_name: Firefox
146:36.36 INFO: application_repository: https://hg.mozilla.org/integration/mozilla-inbound
146:36.36 INFO: application_version: 50.0a1
Was this inbound build good, bad, or broken? (type 'good', 'bad', 'skip', 'retry', 'back' or 'exit' and press Enter): good
149:35.07 INFO: Narrowed inbound regression window from [1983612a, 586815d8] (10 revisions) to [59d49cef, 586815d8] (5 revisions) (~2 steps left)
149:35.07 INFO: Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=59d49cef6561960e4169daa19f901a24c57abdd5&tochange=586815d86b50e44a1cc48c734c4da6efcd8ae8ff

149:35.07 INFO: Downloading build from: https://queue.taskcluster.net/v1/task/Izl1C88mTe2RymVREU9sew/runs/0/artifacts/public%2Fbuild%2Ftarget.tar.bz2
===== Downloaded 100% =====
162:14.08 INFO: Running mozilla-inbound build built on 2016-07-14 13:44:40.396000, revision 481501a8
162:26.97 INFO: Launching /tmp/tmpIWYOyl/firefox/firefox
162:26.97 INFO: Application command: /tmp/tmpIWYOyl/firefox/firefox -profile /tmp/tmpp2Xb_e.mozrunner
162:27.01 INFO: application_buildid: 20160714125214
162:27.01 INFO: application_changeset: 481501a8258f120e980987dd1ec237abc53ded18
162:27.01 INFO: application_name: Firefox
162:27.01 INFO: application_repository: https://hg.mozilla.org/integration/mozilla-inbound
162:27.01 INFO: application_version: 50.0a1
Was this inbound build good, bad, or broken? (type 'good', 'bad', 'skip', 'retry', 'back' or 'exit' and press Enter): good
163:39.00 INFO: Narrowed inbound regression window from [59d49cef, 586815d8] (5 revisions) to [481501a8, 586815d8] (3 revisions) (~1 steps left)
163:39.00 INFO: Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=481501a8258f120e980987dd1ec237abc53ded18&tochange=586815d86b50e44a1cc48c734c4da6efcd8ae8ff

163:39.00 INFO: Downloading build from: https://queue.taskcluster.net/v1/task/AcM72bTJQo2AZ59a1oP0ZQ/runs/0/artifacts/public%2Fbuild%2Ftarget.tar.bz2
===== Downloaded 37% =====

Comment 4

[Loic]

Thanks, it's useful.
The regression range shows a good candidate for your issue:
Andrew Comminos — Bug 1285561 - Refactor surface blitting on X11. r=jrmuizel

Comment 5

[Loic]

what's the value for gfx.xrender.enabled in about:config?
COuld you set to false, restart FF and test again.

Comment 6

[Lee McFarland]

For now my standard FF is 49.0 (because it works).  It seems that Night---  is really test versions of FF.
Is there a way for me to invoke one of them without actually uninstalling my 'production' version for the test,
especially since now I know anything after 5/17 is going to give the error?

Comment 7

[Loic]

Yes, you can use Mozregression for that:
mozregression --launch=2016-05-17 or 49/50/51 etc.
It will use a TEMP profile which is erased after closing the browser.

You can even create a testing profile with some prefs and use it with the command --profile=path_to_profile.

Comment 8

[Loic]

Andrew, could you look at this regression, please. If you have no time, NI? someone else from the graphics team.

Comment 9

[Lee McFarland]

Nightly from 7/17 (which comes up with the bug) has gfx.xrender.enabled set to false already

For future reference, Can I restart that same copy of FF downloaded with mozregression after setting 
a value such as gfx.xrender.enabled to false?  Should you need me to reset a value and restart.

Comment 10

[Loic]

Yes, you can add the command --pref "gfx.xrender.enabled:true/false".

Comment 12

[Lee McFarland]

The duplicate of this bug (dup is 1320347) was reported on Ubuntu 14.04 so the bug has been seen on more than one version of Ubuntu (I'm using 16.04).

Comment 13

[Lee McFarland]

I have since found that by changing the value of gfx.xrender.enabled from it's default of FALSE to TRUE then restarting Firefox, the problem goes away.  This also fixes the problem in the current aurora version FF52.  In my opinion, this is a work around, not a fix.  

Please consider one of the following corrective actions:

1. Fix the code.

2. From FF50 forward, people using X11 would need to have gfx.xrender.enabled be TRUE.  Change the default of this about:config item to be TRUE.  The general user community should not have to override a value in about:config that they've never seen before if the change is due to an upgrade of Firefox.  A quick test of a Ubuntu environment where X11 isn't being used shows it doesn't matter what gfx.xrender.enabled is set to.  This leads me to conclude making the default value for gfx.xrender.enabled TRUE may be an acceptable solution.

Comment 14

[Loic]

Pierrick, could you confirm switching gfx.xrender.enabled from false to true (restart FF to apply) fixes the issue on your machine?

Comment 15

[Lee Salzman [:lsalzman]]

Can you attach the output of running xdpyinfo here?

Comment 16

[Lee Salzman [:lsalzman]]

(In reply to Lee Salzman [:lsalzman] from comment #15)
> Can you attach the output of running xdpyinfo here?

I should clarify - from within xrdp, I mean.

Comment 17

[Lee McFarland]

Result of xdpyinfo:
-----------------------------------
name of display:    :10.0
version number:    11.0
vendor string:    The XFree86 Project, Inc
vendor release number:    40300000
XFree86 version: 4.3.0
maximum request size:  4194300 bytes
motion buffer size:  256
bitmap unit, bit order, padding:    32, LSBFirst, 32
image byte order:    LSBFirst
number of supported pixmap formats:    2
supported pixmap formats:
    depth 1, bits_per_pixel 1, scanline_pad 32
    depth 24, bits_per_pixel 32, scanline_pad 32
keycode range:    minimum 8, maximum 255
focus:  window 0x3000007, revert to Parent
number of extensions:    24
    BIG-REQUESTS
    DEC-XTRAP
    DOUBLE-BUFFER
    Extended-Visual-Information
    FontCache
    GLX
    LBX
    MIT-SCREEN-SAVER
    MIT-SHM
    MIT-SUNDRY-NONSTANDARD
    RANDR
    RECORD
    SECURITY
    SGI-GLX
    SHAPE
    SYNC
    TOG-CUP
    VNC-EXTENSION
    X-Resource
    XC-APPGROUP
    XC-MISC
    XFree86-Bigfont
    XTEST
    XVideo
default screen number:    0
number of screens:    1

screen #0:
  dimensions:    1280x768 pixels (339x203 millimeters)
  resolution:    96x96 dots per inch
  depths (2):    1, 24
  root window id:    0x2e
  depth of root window:    24 planes
  number of colormaps:    minimum 1, maximum 1
  default colormap:    0x21
  default number of colormap cells:    256
  preallocated pixels:    black 0, white 16777215
  options:    backing-store NO, save-unders NO
  largest cursor:    1280x768
  current input event mask:    0xfa800f
    KeyPressMask             KeyReleaseMask           ButtonPressMask          
    ButtonReleaseMask        ExposureMask             StructureNotifyMask      
    SubstructureNotifyMask   SubstructureRedirectMask FocusChangeMask          
    PropertyChangeMask       ColormapChangeMask       
  number of visuals:    8
  default visual id:  0x24
  visual:
    visual id:    0x24
    class:    TrueColor
    depth:    24 planes
    available colormap entries:    256 per subfield
    red, green, blue masks:    0xff, 0xff00, 0xff0000
    significant bits in color specification:    8 bits
  visual:
    visual id:    0x25
    class:    TrueColor
    depth:    24 planes
    available colormap entries:    256 per subfield
    red, green, blue masks:    0xff, 0xff00, 0xff0000
    significant bits in color specification:    8 bits
  visual:
    visual id:    0x26
    class:    TrueColor
    depth:    24 planes
    available colormap entries:    256 per subfield
    red, green, blue masks:    0xff, 0xff00, 0xff0000
    significant bits in color specification:    8 bits
  visual:
    visual id:    0x27
    class:    TrueColor
    depth:    24 planes
    available colormap entries:    256 per subfield
    red, green, blue masks:    0xff, 0xff00, 0xff0000
    significant bits in color specification:    8 bits
  visual:
    visual id:    0x28
    class:    DirectColor
    depth:    24 planes
    available colormap entries:    256 per subfield
    red, green, blue masks:    0xff, 0xff00, 0xff0000
    significant bits in color specification:    8 bits
  visual:
    visual id:    0x29
    class:    DirectColor
    depth:    24 planes
    available colormap entries:    256 per subfield
    red, green, blue masks:    0xff, 0xff00, 0xff0000
    significant bits in color specification:    8 bits
  visual:
    visual id:    0x2a
    class:    DirectColor
    depth:    24 planes
    available colormap entries:    256 per subfield
    red, green, blue masks:    0xff, 0xff00, 0xff0000
    significant bits in color specification:    8 bits
  visual:
    visual id:    0x2b
    class:    DirectColor
    depth:    24 planes
    available colormap entries:    256 per subfield
    red, green, blue masks:    0xff, 0xff00, 0xff0000
    significant bits in color specification:    8 bits

Comment 18

[Lee Salzman [:lsalzman]]

Attachment: allow WindowSurfaceX11Image to render to any visual format

When the WindowSurfaceX11 infrastructure was added, it implemented GetVisualFormat which severely restricted the visual formats we support rendering to. When it fails to map a visual to a SurfaceFormat, it returns SurfaceFormat::UNKNOWN. In release, this still allows rendering to "proceed", but all screwed up. This is likely to happen for VNC/RDP situations.

Here, I modify it so WindowSurfaceX11Image is always used for such scary formats, and that it will always support rendering to them. It does this by first creating a gfxImageSurface in BGRX or BGRA format to render to, then relies upon DrawTargetCairo to blit this out to a gfxXlibSurface, which will implicitly let Cairo do all the nifty swizzling of a sane format to a not-so-sane format.

Comment 19

[Lee McFarland]

(In reply to Lee Salzman [:lsalzman] from comment #16)
> (In reply to Lee Salzman [:lsalzman] from comment #15)
> > Can you attach the output of running xdpyinfo here?
> 
> I should clarify - from within xrdp, I mean.

Yesterday I uploaded the requested information in a comment however, I forgot to do so in a way that resets the 'request for information'

Comment 20

[Lee McFarland]

I just updated 'Nightly' to 53.0a1 (2016-12-01) (64-bit) then switched gfx.xrender.enabled back to the default of FALSE
Upon restart Firefox gets hung up somewhere with only the very top line. There isn't even a 'URL' or command line
where one could get to about:config.  This fix, or whatever went into the just created 'Nightly' build presents a much worse
condition that the original fix.  At least with the bug in the production version (FF50), it is possible to get to about:config.
If this fix was not in 'Nightly' just now, something else is going one.  I was able to get Nightly up only by going to the primary,
non 'X11' console from which I could modify gfx.xrender.enabled to be true.

Comment 21

[Lee Salzman [:lsalzman]]

(In reply to Lee McFarland from comment #20)
> I just updated 'Nightly' to 53.0a1 (2016-12-01) (64-bit) then switched
> gfx.xrender.enabled back to the default of FALSE
> Upon restart Firefox gets hung up somewhere with only the very top line.
> There isn't even a 'URL' or command line
> where one could get to about:config.  This fix, or whatever went into the
> just created 'Nightly' build presents a much worse
> condition that the original fix.  At least with the bug in the production
> version (FF50), it is possible to get to about:config.
> If this fix was not in 'Nightly' just now, something else is going one.  I
> was able to get Nightly up only by going to the primary,
> non 'X11' console from which I could modify gfx.xrender.enabled to be true.

No patch has gone into nightly yet. It is still being reviewed. Please be patient.

Comment 22

[Ryan Hunt [:rhunt]]

Comment on attachment 8816114 [details] [diff] [review]
allow WindowSurfaceX11Image to render to any visual format

>  // Cairo prefers compositing to BGRX instead of BGRA where possible.
>  if (format == gfx::SurfaceFormat::X8R8G8B8_UINT32 &&
>      gfxPlatform::GetPlatform()->GetDefaultContentBackend() != gfx::BackendType::CAIRO) {
>    format = gfx::SurfaceFormat::A8R8G8B8_UINT32;
>  }

Can we use gfxVars::ContentBackend() here instead? That will keep this code GPU process compatible.

Other than that, this looks good to me!

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/47e2650093d5
allow WindowSurfaceX11Image to render to any visual format. r=rhunt

Comment 24

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/47e2650093d5

Comment 25

[Gerry Chang [:gchang]]

Track 51+/52+ as regression.

Comment 26

[Lee Salzman [:lsalzman]]

Comment on attachment 8816114 [details] [diff] [review]
allow WindowSurfaceX11Image to render to any visual format

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1285561
[User impact if declined]: Rendering is distorted or fails entirely in some remote desktop situations.
[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: aurora, beta
[Is the change risky?]: Low-ish risk.
[Why is the change risky/not risky?]: It is mostly concerned with fixing up a rendering fallback that we don't really trigger except in said remote desktop situations. It extends the range of formats we actually support so instead of silently failing to render correctly, we should actually render correctly now.
[String changes made/needed]: None

Comment 27

[Lee McFarland]

According to comment 26 above, the fix has been verified in Nightly.  When would I be able to refresh my 'Nightly' to check out the fix?
Currently I have Nightly 53.0a1 of 12/01/2016 and without gfx.xrender.enabled set TRUE, Firefox won't even come up through my remote connection.

Comment 28

[Lee Salzman [:lsalzman]]

(In reply to Lee McFarland from comment #27)
> According to comment 26 above, the fix has been verified in Nightly.  When
> would I be able to refresh my 'Nightly' to check out the fix?
> Currently I have Nightly 53.0a1 of 12/01/2016 and without
> gfx.xrender.enabled set TRUE, Firefox won't even come up through my remote
> connection.

Patience, please. Wait a couple days.

Comment 29

[Julien Cristau [:jcristau]]

(In reply to Lee McFarland from comment #27)
> According to comment 26 above, the fix has been verified in Nightly.  When
> would I be able to refresh my 'Nightly' to check out the fix?
> Currently I have Nightly 53.0a1 of 12/01/2016 and without
> gfx.xrender.enabled set TRUE, Firefox won't even come up through my remote
> connection.

You should be able to verify with current nightly.

Comment 30

[Lee McFarland]

(In reply to Julien Cristau [:jcristau] from comment #29)
> (In reply to Lee McFarland from comment #27)
> > According to comment 26 above, the fix has been verified in Nightly.  When
> > would I be able to refresh my 'Nightly' to check out the fix?
> > Currently I have Nightly 53.0a1 of 12/01/2016 and without
> > gfx.xrender.enabled set TRUE, Firefox won't even come up through my remote
> > connection.
> 
> You should be able to verify with current nightly.

I am currently running Nightly 2016-12-05  (64-bit).  The problem has been fixed.  I am running with the default value
for gfx.xrender.enabled: False.

The work around until it was fixed was to set gfx.xrender.enabled: TRUE - in this condition, the problem is also fixed.

As further test, I tried Nightly on a non X11 terminal (never was an issue there) - it is also fixed (ie. not broken)

Yes, this problem is fixed.  Thank you for fixing it.

It will be much more important beginning with Firefox 53.  At that point Windows XP users need to find another solution.
Since I cannot move 100% off XP, I'll be running a hybrid where browsing is done from an X11 remote connection to a Ubuntu
system running current Firefox and many other applications. In short, you may see more use of remote(X11 type) connections.

Comment 31

[Loic]

(In reply to Lee McFarland from comment #30)
> It will be much more important beginning with Firefox 53.  At that point
> Windows XP users need to find another solution.
> Since I cannot move 100% off XP, I'll be running a hybrid where browsing is
> done from an X11 remote connection to a Ubuntu
> system running current Firefox and many other applications.

Firefox 52 (and ESR 52) will be the last version to support XP/Vista, so you should start probably to migrate fully to Linux, maybe install a VM if you need to run Win32 applications.

Comment 32

[Julien Cristau [:jcristau]]

marking 53 verified per comment 30

Comment 33

[Julien Cristau [:jcristau]]

Comment on attachment 8816114 [details] [diff] [review]
allow WindowSurfaceX11Image to render to any visual format

fix x11 rendering on some formats seen with xrdp, verified in nightly, take in aurora52

Comment 34

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/34a3558cce8d

Comment 35

[Gerry Chang [:gchang]]

Comment on attachment 8816114 [details] [diff] [review]
allow WindowSurfaceX11Image to render to any visual format

Fix a rendering issue in some remote desktop situations. Beta51+. Should be in 51 beta 7.

Comment 36

[Carsten Book [:Tomcat]]

backed out from beta for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=2067300&repo=mozilla-beta

Comment 37

[Lee Salzman [:lsalzman]]

Attachment: Bug 1319554 - allow WindowSurfaceX11Image to render to any visual format (beta)

Same as above patch that landed in central/aurora, just with reverse-bitrot fixed for beta.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1285561
[User impact if declined]: Rendering is distorted or fails entirely in some remote desktop situations.
[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: beta
[Is the change risky?]: Low-ish risk.
[Why is the change risky/not risky?]: It is mostly concerned with fixing up a rendering fallback that we don't really trigger except in said remote desktop situations. It extends the range of formats we actually support so instead of silently failing to render correctly, we should actually render correctly now.
[String changes made/needed]: None

Comment 38

[Gerry Chang [:gchang]]

Comment on attachment 8818046 [details] [diff] [review]
Bug 1319554 - allow WindowSurfaceX11Image to render to any visual format (beta)

Beta51+. Should be in 51 beta 8.

Comment 40

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/f51416ce7c801f9561098e079681e09b9b3c970e