Closed Bug 946907 Opened 11 years ago Closed 11 years ago

GLContext::ChooseGLFormats uses preferences off the main thread

Categories

(Core :: Graphics, defect)

Product:
Core
Component:
Graphics
Platform:
All
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla30

People

(Reporter: khuey, Assigned: milan)

References

Details

Attachments

(1 file, 4 obsolete files)

Use gfxPrefs to get to the preference on main thread. Carry r=bjacob.
9.46 KB, patch
milan
: review+
Details | Diff | Splinter Review
11:55:57 INFO - 1 XUL!mozilla::Preferences::GetInt(char const*, int*) [Preferences.cpp:c27edc2e1fea : 1332 + 0x4] 11:55:57 INFO - rbx = 0x000000011e8b8fe4 r12 = 0x000000011e8b9064 11:55:57 INFO - r13 = 0x000000011f877800 r14 = 0x00000001045c4280 11:55:58 INFO - r15 = 0x000000011f877ee8 rip = 0x00000001019d6efc 11:55:58 INFO - rsp = 0x000000011e8b8fc0 rbp = 0x000000011e8b8fd0 11:55:58 INFO - Found by: call frame info 11:55:58 INFO - 2 XUL!mozilla::gl::GLContext::ChooseGLFormats(mozilla::gfx::SurfaceCaps const&) const [Preferences.h:c27edc2e1fea : 174 + 0xf] 11:55:58 INFO - rbx = 0x000000011f46e5e0 r12 = 0x000000011e8b9064 11:55:58 INFO - r13 = 0x000000011f877800 r14 = 0x000000011f877800 11:55:58 INFO - r15 = 0x000000011f877ee8 rip = 0x0000000101fd13d8 11:55:58 INFO - rsp = 0x000000011e8b8fe0 rbp = 0x000000011e8b9000 11:55:58 INFO - Found by: call frame info 11:55:58 INFO - 3 XUL!mozilla::gl::GLContext::InitWithPrefix(char const*, bool) [GLContext.h:c27edc2e1fea : 2809 + 0xd] 11:55:58 INFO - rbx = 0x000000011f46e5e0 r12 = 0x000000011e8b9064 11:55:58 INFO - r13 = 0x000000011f877800 r14 = 0x000000011f877ee8 11:55:58 INFO - r15 = 0x000000011f877f70 rip = 0x0000000101fd066f 11:55:58 INFO - rsp = 0x000000011e8b9010 rbp = 0x000000011e8bc1a0 11:55:58 INFO - Found by: call frame info 11:55:58 INFO - 4 XUL!mozilla::gl::GLContextProviderCGL::GetGlobalContext(mozilla::gl::ContextFlags) [GLContextProviderCGL.mm:c27edc2e1fea : 125 + 0x13] 11:55:58 INFO - rbx = 0x0000000000000000 r12 = 0x000000011f242500 11:55:58 INFO - r13 = 0x0000000000000000 r14 = 0x000000011f242500 11:55:58 INFO - r15 = 0x000000011f50aaf0 rip = 0x0000000101fedb6b 11:55:58 INFO - rsp = 0x000000011e8bc1b0 rbp = 0x000000011e8bc1c0 11:55:58 INFO - Found by: call frame info 11:55:58 INFO - 5 XUL!mozilla::gl::GLContextProviderCGL::CreateForWindow(nsIWidget*) [GLContextProviderCGL.mm:c27edc2e1fea : 206 + 0x4] 11:55:58 INFO - rbx = 0x000000011f50aaf0 r12 = 0x000000011f242500 11:55:58 INFO - r13 = 0x0000000000000000 r14 = 0x000000011f242500 11:55:58 INFO - r15 = 0x000000011f50aaf0 rip = 0x0000000101fed4b7 11:55:58 INFO - rsp = 0x000000011e8bc1d0 rbp = 0x000000011e8bc230 11:55:58 INFO - Found by: call frame info 11:55:58 INFO - 6 XUL!mozilla::layers::CompositorOGL::Initialize() [CompositorOGL.cpp:c27edc2e1fea : 273 + 0x8] 11:55:58 INFO - rbx = 0x000000011f50aaf0 r12 = 0x000000011f242500 11:55:58 INFO - r13 = 0x0000000000000000 r14 = 0x000000011f242500 11:55:58 INFO - r15 = 0x000000011f50aaf0 rip = 0x00000001020c68a7 11:55:58 INFO - rsp = 0x000000011e8bc240 rbp = 0x000000011e8bc5f0 11:55:58 INFO - Found by: call frame info 11:55:58 INFO - 7 XUL!mozilla::layers::LayerManagerComposite::Initialize() [LayerManagerComposite.cpp:c27edc2e1fea : 120 + 0x5] 11:55:58 INFO - rbx = 0x000000011f50aaf0 r12 = 0x000000011f242500 11:55:58 INFO - r13 = 0x0000000000000000 r14 = 0x000000011e8bc6f0 11:55:58 INFO - r15 = 0x000000011f50aaf0 rip = 0x000000010209daca 11:55:58 INFO - rsp = 0x000000011e8bc600 rbp = 0x000000011e8bc620 11:55:58 INFO - Found by: call frame info 11:55:58 INFO - 8 XUL!mozilla::layers::CompositorParent::InitializeLayerManager(nsTArray<mozilla::layers::LayersBackend> const&) [CompositorParent.cpp:c27edc2e1fea : 735 + 0x7] 11:55:58 INFO - rbx = 0x000000011f50aaf0 r12 = 0x000000011f242500 11:55:58 INFO - r13 = 0x0000000000000000 r14 = 0x000000011e8bc6f0 11:55:58 INFO - r15 = 0x000000011cd1ff00 rip = 0x00000001020b09e7 11:55:58 INFO - rsp = 0x000000011e8bc630 rbp = 0x000000011e8bc660 11:55:58 INFO - Found by: call frame info 11:55:58 INFO - 9 XUL!mozilla::layers::CompositorParent::AllocPLayerTransactionParent(nsTArray<mozilla::layers::LayersBackend> const&, unsigned long long const&, mozilla::layers::TextureFactoryIdentifier*, bool*) [CompositorParent.cpp:c27edc2e1fea : 754 + 0xa] 11:55:58 INFO - rbx = 0x000000011e8bc6f0 r12 = 0x000000011cd1ff00 11:55:58 INFO - r13 = 0x00000000ffffffff r14 = 0x000000011e8bc6d8 11:55:58 INFO - r15 = 0x000000011e8bc6d7 rip = 0x00000001020b0aee 11:55:58 INFO - rsp = 0x000000011e8bc670 rbp = 0x000000011e8bc6b0 11:55:58 INFO - Found by: call frame info 11:55:58 INFO - 10 XUL!mozilla::layers::PCompositorParent::OnMessageReceived(IPC::Message const&, IPC::Message*&) [PCompositorParent.cpp:c27edc2e1fea : 703 + 0x27] 11:55:58 INFO - rbx = 0x000000011e8bcac8 r12 = 0x000000011e8bcad0 11:55:58 INFO - r13 = 0x00000000ffffffff r14 = 0x000000011e8bca10 11:55:58 INFO - r15 = 0x000000011cd1ff00 rip = 0x0000000101cea980 11:55:58 INFO - rsp = 0x000000011e8bc6c0 rbp = 0x000000011e8bca00 11:55:58 INFO - Found by: call frame info 11:55:58 INFO - 11 XUL!mozilla::ipc::MessageChannel::DispatchSyncMessage(IPC::Message const&) [MessageChannel.cpp:c27edc2e1fea : 918 + 0xc] 11:55:58 INFO - rbx = 0x0000000000000003 r12 = 0x000000011cd1ff60 11:55:58 INFO - r13 = 0x000000011e8bcac8 r14 = 0x000000011e8bcac8 11:55:58 INFO - r15 = 0x000000011cd1ff60 rip = 0x0000000101c8cc4a 11:55:58 INFO - rsp = 0x000000011e8bca10 rbp = 0x000000011e8bca50 11:55:58 INFO - Found by: call frame info 11:55:58 INFO - 12 XUL!mozilla::ipc::MessageChannel::DispatchMessage(IPC::Message const&) [MessageChannel.cpp:c27edc2e1fea : 899 + 0xa] 11:55:58 INFO - rbx = 0x000000011e8bcac8 r12 = 0x000000011cd1ff60 11:55:58 INFO - r13 = 0x000000011e8bcac8 r14 = 0x000000011cd1ff60 11:55:58 INFO - r15 = 0x000000011e8bcaa8 rip = 0x0000000101c8bb56 11:55:58 INFO - rsp = 0x000000011e8bca60 rbp = 0x000000011e8bca90 11:55:58 INFO - Found by: call frame info 11:55:58 INFO - 13 XUL!mozilla::ipc::MessageChannel::OnMaybeDequeueOne() [MessageChannel.cpp:c27edc2e1fea : 890 + 0xa] 11:55:58 INFO - rbx = 0x000000011f467350 r12 = 0x000000011cd1ff60 11:55:58 INFO - r13 = 0x000000011e8bcac8 r14 = 0x000000011f467350 11:55:58 INFO - r15 = 0x000000011e8bcaa8 rip = 0x0000000101c86a7a 11:55:58 INFO - rsp = 0x000000011e8bcaa0 rbp = 0x000000011e8bcb60 11:55:58 INFO - Found by: call frame info 11:55:58 INFO - 14 XUL!MessageLoop::DeferOrRunPendingTask(MessageLoop::PendingTask const&) [message_loop.cc:c27edc2e1fea : 340 + 0x8] 11:55:58 INFO - rbx = 0x000000011e8bccf8 r12 = 0x000000011eaaf7c8 11:55:58 INFO - r13 = 0x000000011eaaf7b0 r14 = 0x000000011f467800 11:55:58 INFO - r15 = 0x000000011e8bcbb0 rip = 0x0000000101c6ea79 11:55:58 INFO - rsp = 0x000000011e8bcb70 rbp = 0x000000011e8bcba0 11:55:58 INFO - Found by: call frame info 11:55:58 INFO - 15 XUL!MessageLoop::DoWork() [message_loop.cc:c27edc2e1fea : 448 + 0xa] 11:55:58 INFO - rbx = 0x000000011e8bccf8 r12 = 0x000000011eaaf7c8 11:55:58 INFO - r13 = 0x000000011eaaf7b0 r14 = 0x000000011e8bcbb8 11:55:58 INFO - r15 = 0x000000011e8bcbb0 rip = 0x0000000101c6ed8a 11:55:58 INFO - rsp = 0x000000011e8bcbb0 rbp = 0x000000011e8bcbe0 11:55:58 INFO - Found by: call frame info 11:55:58 INFO - 16 XUL!base::MessagePumpDefault::Run(base::MessagePump::Delegate*) [message_pump_default.cc:c27edc2e1fea : 34 + 0x8] 11:55:58 INFO - rbx = 0x000000011e8bcc18 r12 = 0x000000011eaaf7c8 11:55:58 INFO - r13 = 0x000000011eaaf7b0 r14 = 0x000000011e8bccf8 11:55:58 INFO - r15 = 0x000000011e8bcc20 rip = 0x0000000101c717f1 11:55:58 INFO - rsp = 0x000000011e8bcbf0 rbp = 0x000000011e8bcc70 11:55:58 INFO - Found by: call frame info 11:55:58 INFO - 17 XUL!MessageLoop::Run() [message_loop.cc:c27edc2e1fea : 222 + 0x8] 11:55:58 INFO - rbx = 0x000000011e8bccf8 r12 = 0x000000011e8bcdf8 11:55:58 INFO - r13 = 0x000000000000d007 r14 = 0x000000011e8bccf8 11:55:58 INFO - r15 = 0x000000011f2d9418 rip = 0x0000000101c6e3a6 11:55:58 INFO - rsp = 0x000000011e8bcc80 rbp = 0x000000011e8bccc0 11:55:58 INFO - Found by: call frame info
Flags: needinfo?(milan)
Assignee: nobody → milan
Flags: needinfo?(milan)
This is waiting for bug 912794 to land, I will ask for the review once it does.
Attachment #8375826 - Flags: review?(bgirard)
Attachment #8375826 - Attachment is obsolete: true
Attachment #8375826 - Flags: review?(bgirard)
Attachment #8376288 - Flags: review?(bjacob)
Comment on attachment 8375826 [details] [diff] [review] Use gfxPrefs to make sure preferences are evaluated on the main thread. This still leaves the preference re-evaluated. Review of attachment 8375826 [details] [diff] [review]: ----------------------------------------------------------------- r+ with the following changes made. ::: gfx/thebes/gfxPlatform.cpp @@ +1639,3 @@ > > + int32_t mode = gfxPrefs::CMSMode(); > + if ((mode >= 0) && (mode < eCMSMode_AllCount)) { nit: I don't think there's a good reason to have extra parentheses for a simple expression. @@ +1652,5 @@ > > int > gfxPlatform::GetRenderingIntent() > { > if (gCMSIntent == -2) { Here -2 appears to mean 'I haven't read the property yet'. This is kind of ugly because a user could request -2 but fine because about:config is for power users anyways. maybe we should #define QCMS_INTENT_UNITIALIZE -2. This code should really initialize the preference to -1 (use embedded profile) and validate the range once when reading the preference simplifying this logic greatly. But I'm ok to r+ just with the -2 define if you're not looking to refactor this here. @@ +1656,5 @@ > if (gCMSIntent == -2) { > > /* Try to query the pref system for a rendering intent. */ > + int32_t pIntent = gfxPrefs::CMSRenderingIntent(); > + if (pIntent != -2) { I don't think this 'if' here is needed. If it's not there line 1662 will reject -2 and we will initialize gCMSIntent to -1 and wont evaluate 1656 as true in the future. ::: gfx/thebes/gfxPrefs.h @@ +102,5 @@ > // This is where DECL_GFX_PREFS for each of the preferences should go. > // We will keep these in an alphabetical order to make it easier to see if > // a method accessing a pref already exists. Just add yours in the list. > > + DECL_GFX_PREFS(Live, "gfx.color_management.enablev4", CMSEnableV4, bool, false); Are you planning on later changing these to be Once? For example changing this pref in a running session will ignore CLUT tags from the output profile and current images but will honor them in any new images which will lead to inconsistent results. @@ +103,5 @@ > // We will keep these in an alphabetical order to make it easier to see if > // a method accessing a pref already exists. Just add yours in the list. > > + DECL_GFX_PREFS(Live, "gfx.color_management.enablev4", CMSEnableV4, bool, false); > + DECL_GFX_PREFS(Live, "gfx.color_management.mode", CMSMode, int32_t,- 1); nit: -1
Attachment #8375826 - Flags: review+
Note the comment 5 is a comment on another bug because I mixed up the patches.
Comment on attachment 8376288 [details] [diff] [review] Use gfxPrefs to get to the preference on main thread. Review of attachment 8376288 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/thebes/gfxPrefs.h @@ +102,5 @@ > // This is where DECL_GFX_PREFS for each of the preferences should go. > // We will keep these in an alphabetical order to make it easier to see if > // a method accessing a pref already exists. Just add yours in the list. > > + DECL_GFX_PREFS(Once, "gfx.work-around-driver-bugs", WorkAroundDriverBugs, bool, true); Since DECL_GFX_PREFS is declaring exactly one pref, maybe it should be DECL_GFX_PREF.
Attachment #8376288 - Flags: review?(bjacob) → review+
Attachment #8376288 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/c7da089782cb
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: