Closed Bug 715658 Opened 13 years ago Closed 12 years ago

[Azure] Images aren't properly updated

Categories

(Core :: Graphics, defect)

Product:
Core
Component:
Graphics
Platform:
x86_64
Windows 7
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: jmjjeffery, Assigned: bas.schouten)

References

(
URL
)

Details

(Keywords: regression)

Attachments

(1 file, 1 obsolete file)

Properly invalidated cached SourceSurfaces v2
19.70 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review 
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.
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.
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.
(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.
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!
Summary: Paint issues and Chrome disappeared → [Azure] Paint issues and Chrome disappeared
(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.
@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.
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.
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.
(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.)
Blocks: 715768
No longer blocks: 711063
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.
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!
Summary: [Azure] Paint issues and Chrome disappeared → [Azure] Images aren't properly updated
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?
Assignee: nobody → bas.schouten
Status: NEW → ASSIGNED
Attachment #587540 - Flags: review?(jmuizelaar)
(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?
Attachment #587540 - Flags: review?(jmuizelaar) → review-
(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).
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 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?
This adds the functions as an exposed cairo API.
Attachment #587540 - Attachment is obsolete: true
Attachment #592758 - Flags: review?(jmuizelaar)
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
Attachment #592758 - Flags: review?(jmuizelaar) → review+
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..
https://hg.mozilla.org/mozilla-central/rev/713814f07168

Leaving open for comment 21.
Target Milestone: --- → mozilla12
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 :).
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.
(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.
Target Milestone: mozilla12 → ---
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.
(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 :).
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. :)
Depends on: 722975
(In reply to Octoploid from comment #21)
Filed as bug 722975.
(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 ?
No longer blocks: 715768
Blocks: 715768
Is this still of any Matter? I cannot repro any mentioned Issues on given Sites.
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.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: