Closed Bug 1386080 Opened 5 years ago Closed 5 years ago

Make DrawTarget thread safe RefCountable

Categories

(Core :: Graphics, enhancement)

55 Branch
enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: mchang, Assigned: mchang)

References

Details

Attachments

(1 file, 3 obsolete files)

With OMTP, we hold RefPtrs to DrawTarget off the main thread. This could potentially be the last reference. Make DrawTarget thread safe refcountable.
Attachment #8892230 - Flags: review?(bas)
Attachment #8892230 - Attachment is obsolete: true
Attachment #8892230 - Flags: review?(bas)
Attachment #8892232 - Flags: review?(bas)
Attachment #8892232 - Attachment is obsolete: true
Attachment #8892232 - Flags: review?(bas)
Attachment #8892234 - Flags: review?(bas)
What if the underlying buffer of the DT (say, a shmem) isn't something the DT can hold a threadsafe reference to?
(In reply to Matt Woodrow (:mattwoodrow) from comment #4)
> What if the underlying buffer of the DT (say, a shmem) isn't something the
> DT can hold a threadsafe reference to?

Well, technically the TextureClient owns the underlying buffer of the DT. We keep that alive on the main thread and so the underlying buffer should be released on the main thread. Same with the DT since the TextureClient should own a reference to it as well. Technically we could just hold draw pointers to the DTs with OMTP, but I'd rather not just for safety.
Why aren't we using AtomicRefCounted rather than changing how the whole thing works?
(In reply to Bas Schouten (:bas.schouten) from comment #6)
> Why aren't we using AtomicRefCounted rather than changing how the whole
> thing works?

Hmm one definition is in the js directory and it seems like we use it. The one in MFBT says don't use it [1] and use the macro instead. Is there something else you meant?


[1] http://searchfox.org/mozilla-central/source/mfbt/RefCounted.h#198
Flags: needinfo?(bas)
(In reply to Mason Chang [:mchang] from comment #7)
> (In reply to Bas Schouten (:bas.schouten) from comment #6)
> > Why aren't we using AtomicRefCounted rather than changing how the whole
> > thing works?
> 
> Hmm one definition is in the js directory and it seems like we use it. The
> one in MFBT says don't use it [1] and use the macro instead. Is there
> something else you meant?
> 
> 
> [1] http://searchfox.org/mozilla-central/source/mfbt/RefCounted.h#198

That's what I meant, but since we avoid dependencies on anything but MFBT (and I think we still should), I think AtomicRefCounted, in line with using RefCounted (which you're not 'supposed to use' either), which we do in all other classes in Moz2D, seems like the right way to go.
Flags: needinfo?(bas)
Attachment #8892234 - Attachment is obsolete: true
Attachment #8892234 - Flags: review?(bas)
Attachment #8893953 - Flags: review?(bas)
Attachment #8893953 - Flags: review?(bas) → review+
Pushed by mchang@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/46c587c3e5de
Make DrawTarget thread safe refcountable. r=bas
https://hg.mozilla.org/mozilla-central/rev/46c587c3e5de
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.