Closed Bug 690469 Opened 8 years ago Closed 8 years ago

ShadowThebesLayer Init/Swap API rework

Categories

(Core :: Graphics, defect)

x86
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla10

People

(Reporter: romaxa, Assigned: romaxa)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

Followup for bug 689045.
Idea is to allocate buffer only when it is needed, make ShadowableThebesLayer allocate only back buffer when it is null, and ShadowThebesLayer should destroy front buffer on destroy or backBuffer and front buffer size different
Assignee: nobody → romaxa
Status: NEW → ASSIGNED
Attachment #563505 - Flags: review?(jones.chris.g)
Comment on attachment 563505 [details] [diff] [review]
ShadowThebesLayer Init/Swap API rework

>+  // Sync front/back buffers content
>+  virtual void SyncData() {}
>+

This name isn't very descriptive.  Please change it to something like
|SyncFrontBufferToBackBuffer)|.

>+  PRPackedBool mNeedSyncData;

Also please change this to something like |mFrontAndBackBufferDiffer|.

>   // This code relies on Swap() arriving *after* attribute mutations.
>-  aNewBack->buffer() = mFrontBufferDescriptor;
>+  if (IsSurfaceDescriptorValid(mFrontBufferDescriptor)) {
>+    *aNewBack = ThebesBuffer();

This isn't necessary.

>+    aNewBack->get_ThebesBuffer().buffer() = mFrontBufferDescriptor;
>+  } else {
>+    NS_WARNING("Front is not valid, set back buffer to null_t");

This shouldn't be a warning, it's part of normal operation when the surface size changes.

It feels to me like this patch should be simpler, but I don't have any
better ideas.

r=me with the fixes above.
Attachment #563505 - Flags: review?(jones.chris.g) → review+
Here is updated patch, but I found some reftest failures on try server...
https://tbpl.mozilla.org/?tree=Try&rev=63618b397502

will check what else is wrong here.
Attachment #563505 - Attachment is obsolete: true
> >+  if (IsSurfaceDescriptorValid(mFrontBufferDescriptor)) {
> >+    *aNewBack = ThebesBuffer();
> 
> This isn't necessary.
actually it is, because before swap call newBack is set to OptionalThebesBuffer, and that is not initialized as ThebesBuffer...

and we will have this crash
REFTEST TEST-START | file://content/base/crashtests/340733-1.html | 18 / 98 (18%)
++DOMWINDOW == 40 (0xaf0e75a8) [serial = 40] [outer = 0xb3501a10]
###!!! ABORT: unexpected type tag: '(mType) == (aType)', file ../../ipc/ipdl/_ipdlheaders/mozilla/layers/PLayers.h, line 1194
mozilla::layers::OptionalThebesBuffer::AssertSanity(mozilla::layers::OptionalThebesBuffer::Type) const (obj-ff-gtk-ds/ipc/ipdl/../../ipc/ipdl/_ipdlheaders/mozilla/layers/PLayers.h:1194)
mozilla::layers::BasicShadowThebesLayer::Swap(mozilla::layers::ThebesBuffer const&, nsIntRegion const&, mozilla::layers::OptionalThebesBuffer*, nsIntRegion*, mozilla::layers::OptionalThebesBuffer*, nsIntRegion*) (gfx/layers/basic/BasicLayers.cpp:2788)
.L186 (gfx/layers/ipc/ShadowLayersParent.cpp:331)
mozilla::layers::PLayersParent::OnMessageReceived(IPC::Message const&, IPC::Message*&) (obj-ff-gtk-ds/ipc/ipdl/PLayersParent.cpp:220)
mozilla::dom::PContentParent::OnMessageReceived(IPC::Message const&, IPC::Message*&) (obj-ff-gtk-ds/ipc/ipdl/PContentParent.cpp:1384)
Blocks: 691417
https://tbpl.mozilla.org/?tree=Try&rev=b81528ed2ba1
Ok, checked bunch of try's and it seems all oranges which are happening in my builds also available in others...
So it should be good enough to checkin
Attachment #564207 - Attachment is obsolete: true
Keywords: checkin-needed
Ah yes, you're right.  IPDL unions don't get a default value.  My fault.
https://hg.mozilla.org/mozilla-central/rev/703df62e855b
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Depends on: 693282
See bug 693086. Causes very frequent crashes of the content process on Fennec.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
The backout was backed out until we can figure out which of several patches broke Android tests:
https://hg.mozilla.org/mozilla-central/rev/bcf12565b95b

Leaving this bug open, as we still plan to back it out ASAP.
Target Milestone: mozilla10 → ---
Re-landed the backout (with a clobber to prevent the test failures):
https://hg.mozilla.org/mozilla-central/rev/b0e79255fa97
Depends on: 693086
This patch need to be landed in couple with patch from bug 690469
in order 
1-st bug 690469, second 693282
https://tbpl.mozilla.org/?tree=Try&rev=5afde775820a
Keywords: checkin-needed
This was re-landed on inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a5b88a4500aa
Keywords: checkin-needed
Whiteboard: [inbound]
Target Milestone: --- → mozilla10
https://hg.mozilla.org/mozilla-central/rev/a5b88a4500aa
Status: REOPENED → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Depends on: 694140
Depends on: 694964
You need to log in before you can comment on or make changes to this bug.