ShadowThebesLayer Init/Swap API rework

RESOLVED FIXED in mozilla10

Status

()

Core
Graphics
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: romaxa, Assigned: romaxa)

Tracking

(Blocks: 1 bug)

Trunk
mozilla10
x86
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

6 years ago
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)

Comment 1

6 years ago
Created attachment 563505 [details] [diff] [review]
ShadowThebesLayer Init/Swap API rework
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+
(Assignee)

Comment 3

6 years ago
Created attachment 564207 [details] [diff] [review]
ShadowThebesLayer Init/Swap API rework

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
(Assignee)

Comment 4

6 years ago
> >+  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)
(Assignee)

Updated

6 years ago
Blocks: 691417
(Assignee)

Comment 5

6 years ago
try build 
https://tbpl.mozilla.org/?tree=Try&rev=d7cebf6100db
(Assignee)

Comment 6

6 years ago
Created attachment 564474 [details] [diff] [review]
ShadowThebesLayer Init/Swap API rework

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
(Assignee)

Updated

6 years ago
Keywords: checkin-needed
Ah yes, you're right.  IPDL unions don't get a default value.  My fault.
http://hg.mozilla.org/integration/mozilla-inbound/rev/703df62e855b
Keywords: checkin-needed
Target Milestone: --- → mozilla10

Comment 9

6 years ago
https://hg.mozilla.org/mozilla-central/rev/703df62e855b
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
(Assignee)

Updated

6 years ago
Depends on: 693282
See bug 693086. Causes very frequent crashes of the content process on Fennec.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
backed out due to crashes reported in bug 693086 https://hg.mozilla.org/mozilla-central/rev/524a6bb8744b
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
(Assignee)

Comment 14

6 years ago
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
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]

Updated

6 years ago
Depends on: 694140

Updated

6 years ago
Depends on: 694964
You need to log in before you can comment on or make changes to this bug.