Last Comment Bug 715658 - [Azure] Images aren't properly updated
: [Azure] Images aren't properly updated
Status: RESOLVED FIXED
: regression
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: Trunk
: x86_64 Windows 7
: -- normal with 2 votes (vote)
: ---
Assigned To: Bas Schouten (:bas.schouten)
:
Mentors:
http://scoop.today.msnbc.msn.com/_new...
: 718197 (view as bug list)
Depends on: 722975
Blocks: 715768
  Show dependency treegraph
 
Reported: 2012-01-05 13:42 PST by Jim Jeffery not reading bug-mail 1/2/11
Modified: 2012-06-26 22:53 PDT (History)
20 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Properly invalidated cached SourceSurfaces (2.70 KB, patch)
2012-01-10 16:57 PST, Bas Schouten (:bas.schouten)
jmuizelaar: review-
Details | Diff | Splinter Review
Properly invalidated cached SourceSurfaces v2 (19.70 KB, patch)
2012-01-30 09:36 PST, Bas Schouten (:bas.schouten)
jmuizelaar: review+
Details | Diff | Splinter Review

Description Jim Jeffery not reading bug-mail 1/2/11 2012-01-05 13:42:34 PST
Added hidden pref: gfx.content.azure.enabled and set to true for testing.

Visiting the URL shows only a partial paint of the top image and successive refreshes on the page show less and less of the image. 

I then entered about:config and toggled the pref back to false.  I lost the Chrome, Nighly button, the windows buttons on top right, along with the addon-bar at the bottom of the page.  Tabs were still displayed. 

I then just closed the browser and restarted and all back to normal but the pref still set to false.  I've not tried yet to see if this is 100% reproducible.
Comment 1 Jim Jeffery not reading bug-mail 1/2/11 2012-01-05 13:48:18 PST
Confirmed it is reproducible.  Toggling the pref on the fly results in odd behavior.  

I did not lose the buttons at the top right, but do lose the Nighly button the enter navbar, and clicking on tabs causes them to disappear.  They are NOT totally lost, just not displayed as they return on restart of the browser.
Comment 2 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-01-05 13:51:29 PST
I don't think dynamic changes to this pref have to work. That would be loads of implementation effort for little gain. Please restart between pref changes.
Comment 3 Jim Jeffery not reading bug-mail 1/2/11 2012-01-05 13:57:50 PST
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #2)
> I don't think dynamic changes to this pref have to work. That would be loads
> of implementation effort for little gain. Please restart between pref
> changes.

OK, but the partial-paint on the image is there even after restarting when visiting the page in the URL.
Comment 4 Bas Schouten (:bas.schouten) 2012-01-05 15:50:46 PST
Dynamic changing will work for a new window (i.e. new layer manager), within the old window it might cause some issues. I made it possible to switch dynamically for easier debugging.

But this is probably a valid bug! Thanks a lot for looking!
Comment 5 Leman Bennett [Omega] 2012-01-05 21:17:13 PST
(In reply to Jim Jeffery not reading bug-mail 1/2/11 from comment #3)
> OK, but the partial-paint on the image is there even after restarting when
> visiting the page in the URL.

I now see this at random while surfing. Some images won't completely display. But will do so after switching tabs for a few seconds. My guess is that images discarding is making them display correctly.
Comment 6 Jim Jeffery not reading bug-mail 1/2/11 2012-01-06 05:15:34 PST
@Omega X,  Yes, the image will repaint after switching tabs, I just now tested and the image does indeed fill-in after several seconds, (10-15 secs maybe).

Also, I find that with HWA 'Off' I get no paint issues on images.

Toggling the pref with HWA 'off' also does not destroy the Chrome.
Comment 7 Michael 2012-01-07 09:41:06 PST
I don't know if this helps, but I noticed that http://www.infowars.com/ has revolving news articles which paint the second line of the article perfectly, but not the first line. Also happens on both 32 and 64bit nightlies. This is on Win7x64SP1. I too have HW accel. on.
Comment 8 Michael 2012-01-07 11:25:32 PST
If you load http://www.kitco.com/charts/livegold.html or http://downloads.guru3d.com/Videocards---ATI-Catalyst-Vista---Win-7_c31.html upon launching the browser, neither one will paint images correctly. Open a new tab and close the previous tab. Wait about 20-30 seconds. Reload either of the two urls and they paint correctly.
Comment 9 Leman Bennett [Omega] 2012-01-07 15:01:10 PST
(In reply to Michael from comment #7)
> I don't know if this helps, but I noticed that http://www.infowars.com/ has
> revolving news articles which paint the second line of the article
> perfectly, but not the first line. Also happens on both 32 and 64bit
> nightlies. This is on Win7x64SP1. I too have HW accel. on.


This is similar to what I've seen with certain text and images with transparency losing their background. The same thing that is happening to JPEGs not displaying completely is probably happening to animated images such as GIFs and APNGs not displaying all of their frames. (I bet that those non displaying JPEGs are progressive.)
Comment 10 :Felipe Gomes (needinfo me!) 2012-01-10 15:30:02 PST
I can reproduce the image issue with this URL: http://en.wikipedia.org/wiki/File:M_Ward_Glastonbury_2009-1.jpg

It doesn't always happen but force refreshing it a few times is usually enough. The image will only partially display and the missing part will be black.
Comment 11 Bas Schouten (:bas.schouten) 2012-01-10 16:54:18 PST
Since live switching isn't really supported. I'm making this bug about progressive JPEGs and animated images not updating properly. Please open separate bugs for any other issues! Thanks!
Comment 12 Bas Schouten (:bas.schouten) 2012-01-10 16:57:27 PST
Created attachment 587540 [details] [diff] [review]
Properly invalidated cached SourceSurfaces

So, this is caused by us keeping around an old SourceSurface for an image, even when the image its based upon has been updated. Cairo has a snapshot API for detecting these changes, but sadly this is an internal API. Attached is a patch that addresses the issue, but it's a little bit hacky in that it imports some of Cairo's internal API. I still think this is the cleanest solution though. What do you think, Jeff?
Comment 13 Jeff Muizelaar [:jrmuizel] 2012-01-11 11:12:49 PST
(In reply to Bas Schouten (:bas) from comment #12)
> Created attachment 587540 [details] [diff] [review]
> Properly invalidated cached SourceSurfaces
> 
> So, this is caused by us keeping around an old SourceSurface for an image,
> even when the image its based upon has been updated. Cairo has a snapshot
> API for detecting these changes, but sadly this is an internal API. Attached
> is a patch that addresses the issue, but it's a little bit hacky in that it
> imports some of Cairo's internal API. I still think this is the cleanest
> solution though. What do you think, Jeff?

Not thrilled. Can't we put something in imgFrame::UnlockImageData() instead?
Comment 14 Bas Schouten (:bas.schouten) 2012-01-11 16:21:38 PST
(In reply to Jeff Muizelaar [:jrmuizel] from comment #13)
> (In reply to Bas Schouten (:bas) from comment #12)
> > Created attachment 587540 [details] [diff] [review]
> > Properly invalidated cached SourceSurfaces
> > 
> > So, this is caused by us keeping around an old SourceSurface for an image,
> > even when the image its based upon has been updated. Cairo has a snapshot
> > API for detecting these changes, but sadly this is an internal API. Attached
> > is a patch that addresses the issue, but it's a little bit hacky in that it
> > imports some of Cairo's internal API. I still think this is the cleanest
> > solution though. What do you think, Jeff?
> 
> Not thrilled. Can't we put something in imgFrame::UnlockImageData() instead?

I don't think that's a good idea, that would mean the wrapper and its behavior are still fundamentally broken. Which is a problem, the alternatives would be simply always re-uploading, which I think we can agree will cause terrible performance problems. Or keeping a list like snapshots inside gfxASurface, but that just seems like silly duplication of functionality (trivial, but 3 times as many lines of code with no real advantage other than some code duplication).
Comment 15 Bryan Price 2012-01-12 20:56:58 PST
I experienced this bug.  At first I thought it was an extension update, as my other Nightly was not experiencing it (has quite a lot less in extensions), but I was able to go back and figure out what I had done.  It was quite wild losing everything after setting it off (I lost tabs, menu, bookmark bar, everything), although it recovered after restarting.

I knew something was off when I could start dragging the image and could see the complete image.
Comment 16 Jeff Muizelaar [:jrmuizel] 2012-01-14 10:37:03 PST
Comment on attachment 587540 [details] [diff] [review]
Properly invalidated cached SourceSurfaces

After thinking it over I'm still not thrilled with this approach, but I tend to agree that this is probably the best way to ensure we get the correct semantics with out introducing subtle bugs. I would be ok to take it if you turned it into a proper cairo patch that adds the API that you need to cairo. Does that work for you?
Comment 17 Andrew McCreight [:mccr8] 2012-01-17 09:58:36 PST
*** Bug 718197 has been marked as a duplicate of this bug. ***
Comment 18 Bas Schouten (:bas.schouten) 2012-01-30 09:36:15 PST
Created attachment 592758 [details] [diff] [review]
Properly invalidated cached SourceSurfaces v2

This adds the functions as an exposed cairo API.
Comment 19 Jeff Muizelaar [:jrmuizel] 2012-01-30 19:20:05 PST
Comment on attachment 592758 [details] [diff] [review]
Properly invalidated cached SourceSurfaces v2

Review of attachment 592758 [details] [diff] [review]:
-----------------------------------------------------------------

Creating a public wrapper around the existing private function would have made the diff smaller but it's not a big deal. Make sure you add the patch to the tree and README
Comment 20 Bas Schouten (:bas.schouten) 2012-01-30 21:17:32 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/713814f07168
Comment 21 Octoploid 2012-01-31 05:46:37 PST
This breaks --enable-default-toolkit=cairo-gtk2 --enable-system-cairo:

/var/tmp/mozilla-central/gfx/thebes/gfxPlatform.cpp: In member function ‘virtual mozilla::RefPtr<mozilla::gfx::SourceSurface> gfxPlatform::GetSourceS
urfaceForSurface(mozilla::gfx::DrawTarget*, gfxASurface*)’:
/var/tmp/mozilla-central/gfx/thebes/gfxPlatform.cpp:516:53: error: ‘cairo_null_surface_create’ was not declared in this scope
/var/tmp/mozilla-central/gfx/thebes/gfxPlatform.cpp:521:95: error: ‘cairo_surface_attach_snapshot’ was not declared in this scope
make[6]: *** [gfxPlatform.i_o] Error 1
make[6]: *** Waiting for unfinished jobs..
Comment 22 Ed Morley [:emorley] 2012-01-31 06:50:33 PST
https://hg.mozilla.org/mozilla-central/rev/713814f07168

Leaving open for comment 21.
Comment 23 Bas Schouten (:bas.schouten) 2012-01-31 07:51:45 PST
Jeff, what should we do here? I forgot all about system cairo (and I guess so did you :)) when I decided to make API changes :).
Comment 24 thewolf86 2012-01-31 08:07:46 PST
I'm using rev. 3f26b7bee352 so I'm sure this patch is in. 

While images load more frequently I'm still seeing GIFs not playing and images not displaying including the examples in this bug. Sorry, if I'm speaking prematurely, and there are more parts yet to be landed.
Comment 25 Jim Jeffery not reading bug-mail 1/2/11 2012-01-31 08:30:44 PST
(In reply to thewolf86 from comment #24)
> I'm using rev. 3f26b7bee352 so I'm sure this patch is in. 
> 
> While images load more frequently I'm still seeing GIFs not playing and
> images not displaying including the examples in this bug. Sorry, if I'm
> speaking prematurely, and there are more parts yet to be landed.

No, your build is 'before' the bug landed, and will be in tomorrows nightly.  
The bug landed in cset: https://hg.mozilla.org/mozilla-central/rev/060de47a1822

Its in the latest hourly, but using that will take you off the nightly update channel, so you'd likely be better off just waiting.  There is also the possibility it will be backed I would guess due to comment #21 and following.
Comment 26 thewolf86 2012-01-31 09:55:36 PST
Yes, yes... I'm aware of that. I mentioned the cset 3f26b7bee352 specifically because I'm using an hourly. On the Tinderbox pushlog 3f26b7bee352 comes after this landed in mozilla-inbound. 

I've been off of the nightly update channel for ages now. 99% of the updates are irrelevant to what interests me... unless it indirectly breaks something I am interested in.
Comment 27 Bas Schouten (:bas.schouten) 2012-01-31 11:41:52 PST
(In reply to thewolf86 from comment #26)
> Yes, yes... I'm aware of that. I mentioned the cset 3f26b7bee352
> specifically because I'm using an hourly. On the Tinderbox pushlog
> 3f26b7bee352 comes after this landed in mozilla-inbound. 
> 
> I've been off of the nightly update channel for ages now. 99% of the updates
> are irrelevant to what interests me... unless it indirectly breaks something
> I am interested in.

That cset was landed directly on mozilla-central though, my changeset was merged to mozilla-central -after- that changeset landed on m-c :).
Comment 28 thewolf86 2012-01-31 11:56:16 PST
Ahh... That explains it. My mistake: There was some issue with the http version of the ftp site where it would not list contents of subdirectories under the Firefox folder so I browsed there manually instead of using my bookmarks - must've ended up in m-c instead of m-i by accident. Latest builds works as advertised. I like you Bas. :)
Comment 29 Takanori MATSUURA 2012-02-02 01:26:09 PST
(In reply to Octoploid from comment #21)
Filed as bug 722975.
Comment 30 Jim Jeffery not reading bug-mail 1/2/11 2012-03-22 03:58:20 PDT
(In reply to Ed Morley [:edmorley] from comment #22)
> https://hg.mozilla.org/mozilla-central/rev/713814f07168
> 
> Leaving open for comment 21.

The issue now has its own bug 722975 ?  Any reason this can't be closed now ?
Comment 31 XtC4UaLL [:xtc4uall] 2012-06-25 17:23:00 PDT
Is this still of any Matter? I cannot repro any mentioned Issues on given Sites.
Comment 32 Jim Jeffery not reading bug-mail 1/2/11 2012-06-25 17:44:14 PDT
Commnet #21 has its own bug now per comment #30, and I've not seen this paint issue since the patch landed. 

Closing.  File new bugs or reopen as needed.

Note You need to log in before you can comment on or make changes to this bug.