Closed
Bug 1094338
Opened 10 years ago
Closed 10 years ago
WebGL rendering errors in UE4 strategy game demo on 10.8 or less because of unsupported WEBGL_depth_texture on those systems
Categories
(Core :: Graphics, defect)
Tracking
()
RESOLVED
FIXED
mozilla36
People
(Reporter: nick, Assigned: milan)
References
Details
(Whiteboard: webgl-site)
Attachments
(3 files, 4 obsolete files)
Running FF 33.0.2 OSX 10.8.5. Getting repeating errors in the console: Error: WebGL: texImage2D: invalid format DEPTH_COMPONENT: need WEBGL_depth_texture enabled StrategyGame-HTML5-Shipping.js:1 Error: WebGL: clear: incomplete framebuffer See attached screenshot http://ue4.mozilla.org.s3-website-us-east-1.amazonaws.com/
Reporter | ||
Comment 1•10 years ago
|
||
From http://webglreport.com/ I see my supported extensions: ANGLE_instanced_arrays EXT_frag_depth EXT_sRGB EXT_texture_filter_anisotropic OES_element_index_uint OES_standard_derivatives OES_texture_float OES_texture_float_linear OES_texture_half_float OES_texture_half_float_linear OES_vertex_array_object WEBGL_compressed_texture_s3tc WEBGL_draw_buffers WEBGL_lose_context MOZ_WEBGL_lose_context MOZ_WEBGL_compressed_texture_s3tc
Reporter | ||
Comment 2•10 years ago
|
||
from webgl report "To see draft extensions in Firefox, browse to about:config and set webgl.enable-draft-extensions to true." If I reload, I then get: ANGLE_instanced_arrays EXT_blend_minmax EXT_color_buffer_half_float EXT_frag_depth EXT_sRGB EXT_texture_filter_anisotropic OES_element_index_uint OES_standard_derivatives OES_texture_float OES_texture_float_linear OES_texture_half_float OES_texture_half_float_linear OES_vertex_array_object WEBGL_color_buffer_float WEBGL_compressed_texture_s3tc WEBGL_draw_buffers WEBGL_lose_context MOZ_WEBGL_lose_context MOZ_WEBGL_compressed_texture_s3tc
Assignee | ||
Comment 3•10 years ago
|
||
Did this ever work, is it a recent regression?
Reporter | ||
Comment 4•10 years ago
|
||
This is my first time trying it today. Not sure how old the error is. MSchifer has been able to reproduce as well. I'm curious if this is reproducible on newer versions of OSX. From my earlier posts, I indeed do not have the webgl extension that's listed in the error `WEBGL_depth_texture`, though Vlad mentioned that the issue may not be related.
Comment 5•10 years ago
|
||
Using Aurora (35.0a2 (2014-11-02)) on 10.10, MacBook Pro (Retina, 15-inch, Early 2013) the rendering is OK. I do experience 30s+ unresponsive browser during loading and severe jank during the game. Extensions: ANGLE_instanced_arrays EXT_blend_minmax EXT_frag_depth EXT_sRGB EXT_texture_filter_anisotropic OES_element_index_uint OES_standard_derivatives OES_texture_float OES_texture_float_linear OES_texture_half_float OES_texture_half_float_linear OES_vertex_array_object WEBGL_compressed_texture_s3tc WEBGL_depth_texture WEBGL_draw_buffers WEBGL_lose_context MOZ_WEBGL_lose_context MOZ_WEBGL_compressed_texture_s3tc MOZ_WEBGL_depth_texture Extensions including draft: ANGLE_instanced_arrays EXT_blend_minmax EXT_color_buffer_half_float EXT_frag_depth EXT_sRGB EXT_texture_filter_anisotropic OES_element_index_uint OES_standard_derivatives OES_texture_float OES_texture_float_linear OES_texture_half_float OES_texture_half_float_linear OES_vertex_array_object WEBGL_color_buffer_float WEBGL_compressed_texture_s3tc WEBGL_depth_texture WEBGL_draw_buffers WEBGL_lose_context MOZ_WEBGL_lose_context MOZ_WEBGL_compressed_texture_s3tc MOZ_WEBGL_depth_texture
Assignee | ||
Comment 6•10 years ago
|
||
On the off chance that I don't know what "UE4 strategy game demo" is, can we get some details?
Reporter | ||
Comment 7•10 years ago
|
||
With FF 36.0a1 (2014-11-02) OSX 10.8.5, I only see: Error: WebGL: texImage2D: invalid format DEPTH_COMPONENT: need WEBGL_depth_texture enabled StrategyGame-HTML5-Shipping.js:1 attached second screenshot, more is rendered, but is still broken.
Reporter | ||
Comment 8•10 years ago
|
||
Reporter | ||
Updated•10 years ago
|
Attachment #8517591 -
Attachment description: Screen Shot 2014-11-05 at 10.32.06 AM.png → FF Release
Reporter | ||
Comment 9•10 years ago
|
||
Also, I don't happen to see the listed extensions [0] in Apple's support table [1]. [0] http://dxr.mozilla.org/mozilla-central/source/dom/canvas/WebGLContextExtensions.cpp#146 [1] https://developer.apple.com/opengl/capabilities/
Reporter | ||
Comment 10•10 years ago
|
||
So more specifically seems like: OSX 10.8.5 release and nightly (possibly beta and aurora) versions of FF affected. OSX 10.10, aurora (possibly other) versions of FF affected.
OS: All → Mac OS X
Hardware: All → x86_64
(In reply to Nick Desaulniers [:\n] from comment #0) > Created attachment 8517591 [details] > FF Release > > Running FF 33.0.2 OSX 10.8.5. Getting repeating errors in the console: On what hardware, especially GPU?
Reporter | ||
Comment 12•10 years ago
|
||
MacBook Pro Retina, mid 2012 NVIDIA GeForce GT 650M: Chipset Model: NVIDIA GeForce GT 650M Type: GPU Bus: PCIe PCIe Lane Width: x8 VRAM (Total): 1024 MB Vendor: NVIDIA (0x10de) Device ID: 0x0fd5 Revision ID: 0x00a2 ROM Revision: 3688 gMux Version: 3.2.19 [3.2.8] Intel HD Graphics 4000: Chipset Model: Intel HD Graphics 4000 Type: GPU Bus: Built-In VRAM (Total): 512 MB Vendor: Intel (0x8086) Device ID: 0x0166 Revision ID: 0x0009 gMux Version: 3.2.19 [3.2.8]
Comment 13•10 years ago
|
||
WEBGL_depth_texture should be universally supported on desktop. We support it with ANGLE, and all desktop GL 2.0+ should have this functionality. Post an about:support.
Flags: needinfo?(nick)
I found a brief period from a 09-29 nightly where it wasn't showing up in the supported extensions list, which I thought was very strange (on Windows). I don't have a 10.8.5 machine to check what's going on there to see if I can duplicate Nick's problem.
Reporter | ||
Comment 15•10 years ago
|
||
Application Basics ------------------ Name: Firefox Version: 36.0a1 User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:36.0) Gecko/20100101 Firefox/36.0 Multiprocess Windows: 0/3 Crash Reports for the Last 3 Days --------------------------------- All Crash Reports Extensions ---------- Name: ADB Helper Version: 0.7.1 Enabled: true ID: adbhelper@mozilla.org Name: Adblock Plus Version: 2.6.5 Enabled: true ID: {d10d0bf8-f5b5-c8b4-a8b2-2b9879e08c5d} Name: ColorZilla Version: 2.8 Enabled: true ID: {6AC85730-7D0F-4de0-B3FA-21142DD85326} Name: Firebug Version: 2.0.6 Enabled: true ID: firebug@software.joehewitt.com Name: Firefox OS 1.3 Simulator Version: 7.0pre9.20140401 Enabled: true ID: fxos_1_3_simulator@mozilla.org Name: Firefox OS 1.4 Simulator Version: 1.4.20140506 Enabled: true ID: fxos_1_4_simulator@mozilla.org Name: Firefox OS 2.0 Simulator Version: 2.0.20140918 Enabled: true ID: fxos_2_0_simulator@mozilla.org Name: Hacker News Colors Version: 0.1 Enabled: true ID: jid1-lwhDTbxS62Tzog@jetpack Name: HTTPS-Everywhere Version: 4.0.2 Enabled: true ID: https-everywhere@eff.org Name: JIT Inspector Version: 0.5.0.0 Enabled: true ID: bhackett@mozilla.com Name: Pocket Version: 3.0.5 Enabled: true ID: isreaditlater@ideashower.com Name: User Agent Switcher Version: 0.7.3 Enabled: true ID: {e968fc70-8f95-4ab9-9e79-304de2a71ee1} Name: Add-on Builder Helper (discontinued) Version: 1.8.final Enabled: false ID: jid0-t3eeRQgGANLCH9c50lPqcTDuNng@jetpack Name: Firefox OS Simulator Version: 4.0.2 Enabled: false ID: r2d2b2g@mozilla.org Name: Gecko Profiler Version: 1.14.2 Enabled: false ID: jid0-edalmuivkozlouyij0lpdx548bc@jetpack Name: Privacy Badger Firefox Version: 0.2.3 Enabled: false ID: jid1-MnnxcxisBPnSXQ@jetpack Name: Shumway Version: 0.9.3211 Enabled: false ID: shumway@research.mozilla.org Name: WebGL-Inspector Version: 1.0.4 Enabled: false ID: jid1-wzJMQZdgCoDRJA@jetpack Name: Youtube MP3 Podcaster Version: 3.3.4 Enabled: false ID: youtubemp3podcaster@jeremy.d.gregorio.com Name: YSlow Version: 3.1.8 Enabled: false ID: yslow@yahoo-inc.com Graphics -------- Device ID: 0x fd5 GPU Accelerated Windows: 3/3 OpenGL (OMTC) Vendor ID: 0x10de WebGL Renderer: NVIDIA Corporation -- NVIDIA GeForce GT 650M OpenGL Engine windowLayerManagerRemote: true AzureCanvasBackend: quartz AzureContentBackend: quartz AzureFallbackCanvasBackend: none AzureSkiaAccelerated: 0 Important Modified Preferences ------------------------------ accessibility.typeaheadfind.flashBar: 0 browser.cache.disk.capacity: 358400 browser.cache.disk.smart_size_cached_value: 358400 browser.cache.disk.smart_size.first_run: false browser.cache.disk.smart_size.use_old_max: false browser.cache.frecency_experiment: 3 browser.places.smartBookmarksVersion: 7 browser.search.useDBForOrder: true browser.sessionstore.upgradeBackup.latestBuildID: 20141105030203 browser.startup.homepage_override.buildID: 20141105030203 browser.startup.homepage_override.mstone: 36.0a1 browser.tabs.warnOnClose: false dom.mozApps.maxLocalId: 1006 dom.mozApps.used: true dom.mozTCPSocket.enabled: true dom.w3c_touch_events.expose: false extensions.lastAppVersion: 36.0a1 gfx.blacklist.webgl.msaa: 4 media.gmp-gmpopenh264.lastUpdate: 1412236168 media.gmp-gmpopenh264.path: /Users/Nicholas/Library/Application Support/Firefox/Profiles/r4z13aww.default/gmp-gmpopenh264 media.gmp-gmpopenh264.version: 1.1 media.gmp-manager.lastCheck: 1415226101 network.cookie.cookieBehavior: 1 network.cookie.prefsMigrated: true network.warnOnAboutNetworking: false places.database.lastMaintenance: 1415300571 places.history.expiration.transient_current_max_pages: 104858 plugin.disable_full_page_plugin_for_types: application/pdf plugin.importedState: true plugin.state.adobepdfviewernpapi: 0 plugin.state.flash: 1 plugin.state.googletalkbrowserplugin: 1 plugin.state.java: 0 plugin.state.npgtpo3dautoplugin: 0 plugin.state.o1dbrowserplugin: 1 plugin.state.silverlight: 1 plugin.state.unity web player: 1 print.print_bgcolor: false print.print_bgimages: false print.print_colorspace: print.print_command: print.print_downloadfonts: false print.print_duplex: 402653184 print.print_evenpages: true print.print_in_color: true print.print_margin_bottom: 0.5 print.print_margin_left: 0.5 print.print_margin_right: 0.5 print.print_margin_top: 0.5 print.print_oddpages: true print.print_orientation: 0 print.print_page_delay: 50 print.print_paper_data: 0 print.print_paper_height: 11.00 print.print_paper_name: print.print_paper_size_type: 1 print.print_paper_size_unit: 0 print.print_paper_width: 8.50 print.print_plex_name: print.print_resolution: 32767 print.print_resolution_name: print.print_reversed: false print.print_scaling: 1.00 print.print_shrink_to_fit: true print.print_to_file: false print.print_unwriteable_margin_bottom: 56 print.print_unwriteable_margin_left: 25 print.print_unwriteable_margin_right: 25 print.print_unwriteable_margin_top: 25 privacy.cpd.cache: false privacy.cpd.cookies: false privacy.cpd.sessions: false privacy.donottrackheader.enabled: true privacy.sanitize.migrateFx3Prefs: true security.warn_viewing_mixed: false storage.vacuum.last.index: 1 storage.vacuum.last.places.sqlite: 1414446785 Important Locked Preferences ---------------------------- JavaScript ---------- Incremental GC: true Accessibility ------------- Activated: false Prevent Accessibility: 0 Library Versions ---------------- NSPR Expected minimum version: 4.10.7 Version in use: 4.10.7 NSS Expected minimum version: 3.18 Basic ECC Beta Version in use: 3.18 Basic ECC Beta NSSSMIME Expected minimum version: 3.18 Basic ECC Beta Version in use: 3.18 Basic ECC Beta NSSSSL Expected minimum version: 3.18 Basic ECC Beta Version in use: 3.18 Basic ECC Beta NSSUTIL Expected minimum version: 3.18 Beta Version in use: 3.18 Beta Experimental Features --------------------- Name: Invisible test of the experiment branching system. ID: experiment-branch-test-nightly@experiments.mozilla.org Description: An experiment using branches just to test whether branches get saved correctly. Active: false End Date: 1409851615004 Homepage:
Flags: needinfo?(nick)
Comment 16•10 years ago
|
||
We're hitting this: #ifdef XP_MACOSX // The Mac Nvidia driver, for versions up to and including 10.8, don't seem // to properly support this. See 814839 // this has been fixed in Mac OS X 10.9. See 907946 if (Vendor() == gl::GLVendor::NVIDIA && !nsCocoaFeatures::OnMavericksOrLater()) { MarkUnsupported(GLFeature::depth_texture); } #endif If we want to re-enable it, we need to narrow the scope of this backlisting.
Reporter | ||
Comment 17•10 years ago
|
||
From stdout from a debug build: OpenGL version detected: 210 OpenGL vendor: NVIDIA Corporation OpenGL renderer: NVIDIA GeForce GT 650M OpenGL Engine [76727] WARNING: depth_texture marked as unsupported: file /builds/slave/m-cen-osx64-d-0000000000000000/build/gfx/gl/GLContextFeatures.cpp, line 740
Comment 18•10 years ago
|
||
(In reply to Nick Desaulniers [:\n] from comment #10) > So more specifically seems like: > > OSX 10.8.5 release and nightly (possibly beta and aurora) versions of FF > affected. > OSX 10.10, aurora (possibly other) versions of FF affected. From comment 5, it sounds like 10.10 is unaffected. Based on the code: WEBGL_depth_texture is unavailable on Mac10.8 or lower for NVidia cards, for all reasonable versions of FF. The demo seems to try to use this extension without checking for it. If we don't support WEBGL_depth_texture, the demo will render incorrectly. Sounds like a demo issue.
Updated•10 years ago
|
Whiteboard: webgl-site
Assignee | ||
Comment 19•10 years ago
|
||
Thanks for the analysis guys! Moving to evangelism.
Component: Canvas: WebGL → Desktop
Product: Core → Tech Evangelism
Summary: WebGL rendering errors in UE4 strategy game demo → WebGL rendering errors in UE4 strategy game demo on 10.8 or less because of unsupported WEBGL_depth_texture on those systems
Version: 30 Branch → Firefox 33
Not quite evang -- according to bug 908905, ARB_depth_texture is broken only on OSX <= 10.8.2. It seems to work fine in 10.8.3 (and presumably in 10.8.5), but we blacklist it on anything < 10.9. So we could fix this in our code as well.
Assignee | ||
Updated•10 years ago
|
Component: Desktop → Graphics
Product: Tech Evangelism → Core
Version: Firefox 33 → 33 Branch
Assignee | ||
Comment 21•10 years ago
|
||
(In reply to Vladimir Vukicevic [:vlad] [:vladv] from comment #20) > Not quite evang -- according to bug 908905, ARB_depth_texture is broken only > on OSX <= 10.8.2. It seems to work fine in 10.8.3 (and presumably in > 10.8.5), but we blacklist it on anything < 10.9. So we could fix this in > our code as well. Except that comment 0 talks about it failing on 10.8.5...
Assignee | ||
Comment 22•10 years ago
|
||
Never mind. Friday afternoon. Duh...
Assignee | ||
Comment 23•10 years ago
|
||
Nick, can you try this patch?
Flags: needinfo?(nick)
Attachment #8519160 -
Flags: review?(vladimir)
Assignee | ||
Comment 24•10 years ago
|
||
Attachment #8519160 -
Attachment is obsolete: true
Attachment #8519160 -
Flags: review?(vladimir)
Attachment #8519163 -
Flags: review?(vladimir)
Comment on attachment 8519163 [details] [diff] [review] Allow depth texture on 10.8.3 and higher instead of holding out for 10.9 In the spirit of Friday, I was going to ask "What if the major is less than 10?" Asking jeff to r+ this because he was involved in the earlier 10.8.2/10.8.3 stuff to make sure this is safe. + if (aMajor< 10) { space before <
Attachment #8519163 -
Flags: review?(vladimir)
Attachment #8519163 -
Flags: review?(jgilbert)
Attachment #8519163 -
Flags: feedback+
Comment 26•10 years ago
|
||
Comment on attachment 8519163 [details] [diff] [review] Allow depth texture on 10.8.3 and higher instead of holding out for 10.9 Review of attachment 8519163 [details] [diff] [review]: ----------------------------------------------------------------- ::: widget/cocoa/nsCocoaFeatures.mm @@ +158,5 @@ > > +/* static */ bool > +nsCocoaFeatures::AtLeastVersion(int32_t aMajor, int32_t aMinor, int32_t aBugFix) > +{ > + if (aMajor< 10) { This early-out conditional is unnecessary if the logic below is done properly. @@ +161,5 @@ > +{ > + if (aMajor< 10) { > + return false; > + } > + return mOSXVersion >= 0x1000 + (aMinor << 4) + aBugFix; Use `OSXVersion{Major,Minor,BugFix}()` as seen above in AccelerateByDefault.
Attachment #8519163 -
Flags: review?(jgilbert) → review-
Reporter | ||
Updated•10 years ago
|
Flags: needinfo?(nick)
Assignee | ||
Comment 27•10 years ago
|
||
Yeah, but that ends up being ugly like: if (OSXVersionMajor() == aMajor) { if (OSXVersionMinor() == aMinor) { return OSXVersionBugFix() >= aBugFix; } else { return OSXVersionMinor() > aMinor; } } else { return OSXVersionMajor() > aMajor; } Anyway, I'll take a look.
Assignee | ||
Comment 28•10 years ago
|
||
Existing code doesn't really work for major version > 10, not that we have cases for that...
Comment 29•10 years ago
|
||
(In reply to Milan Sreckovic [:milan] from comment #28) > Existing code doesn't really work for major version > 10, not that we have > cases for that... Right, but it's strictly less code to ignore that.
Comment 30•10 years ago
|
||
(In reply to Milan Sreckovic [:milan] from comment #27) > Yeah, but that ends up being ugly like: > > if (OSXVersionMajor() == aMajor) { > if (OSXVersionMinor() == aMinor) { > return OSXVersionBugFix() >= aBugFix; > } else { > return OSXVersionMinor() > aMinor; > } > } else { > return OSXVersionMajor() > aMajor; > } > > Anyway, I'll take a look. Or: If (Major() > aMajor) return true; if (Major() < aMajor) return false; if (Minor() > aMinor) return true; if (Minor() < aMinor) return false; return BugFix() >= aBugFix;
Assignee | ||
Comment 31•10 years ago
|
||
Attachment #8519163 -
Attachment is obsolete: true
Attachment #8520367 -
Flags: review?(jgilbert)
Updated•10 years ago
|
Attachment #8520367 -
Flags: review?(jgilbert) → review+
Assignee | ||
Comment 32•10 years ago
|
||
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=d646ab2855f0
Assignee | ||
Comment 33•10 years ago
|
||
I made enough changes that I'll bother you again for a review Jeff. One of the new member functions became non-member function, as it wasn't accessing any member variables. Added a lot more asserts, and extracted the version &, <<, + math into separate utility inlines to keep it cleaner. The previous version had an infinite recursion in the debug build. https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=7ec110e3ff2f
Attachment #8520367 -
Attachment is obsolete: true
Attachment #8521556 -
Flags: review?(jgilbert)
Assignee | ||
Comment 34•10 years ago
|
||
Green try \o/ https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=7ec110e3ff2f
Comment 35•10 years ago
|
||
Comment on attachment 8521556 [details] [diff] [review] Allow depth texture on 10.8.3 and higher instead of holding out for 10.9. Rework the OS X version tracking in the process. r=jgilbert Review of attachment 8521556 [details] [diff] [review]: ----------------------------------------------------------------- ::: widget/cocoa/nsCocoaFeatures.mm @@ +29,5 @@ > #import <Cocoa/Cocoa.h> > > int32_t nsCocoaFeatures::mOSXVersion = 0; > + > +inline int32_t VersionAssemble(int32_t aMajor, int32_t aMinor, int32_t aBugFix) `AssembleVersion` @@ +35,5 @@ > + MOZ_ASSERT(aMajor == 10); > + return MAC_OS_X_VERSION_10_0_HEX + (aMinor << 4) + aBugFix; > +} > + > +inline int32_t VersionExtractMajor(int32_t aVersion) `ExtractMajorVersion` @@ +42,5 @@ > + MOZ_ASSERT((aVersion & MAC_OS_X_VERSION_10_0_HEX) == MAC_OS_X_VERSION_10_0_HEX); > + return 10; > +} > + > +inline int32_t VersionExtractMinor(int32_t aVersion) `ExtractMinorVersion` @@ +48,5 @@ > + MOZ_ASSERT((aVersion & MAC_OS_X_VERSION_MASK) == aVersion); > + return (aVersion & 0xF0) >> 4; > +} > + > +inline int32_t VersionExtractBugFix(int32_t aVersion) `ExtractBugFixVersion` @@ +134,5 @@ > /* static */ int32_t > nsCocoaFeatures::OSXVersionMajor() > { > + MOZ_ASSERT((OSXVersion() & MAC_OS_X_VERSION_10_0_HEX) == MAC_OS_X_VERSION_10_0_HEX); > + return 10; We have a function which does this. There's no reason to hard-code this. @@ +190,5 @@ > > +/* static */ bool > +nsCocoaFeatures::IsAtLeastVersion(int32_t aMajor, int32_t aMinor, int32_t aBugFix) > +{ > + return OSXVersion() >= GetFullVersion(aMajor, aMinor, aBugFix); Did you mean `VersionAssemble`?
Attachment #8521556 -
Flags: review?(jgilbert) → review+
Assignee | ||
Comment 36•10 years ago
|
||
(In reply to Jeff Gilbert [:jgilbert] from comment #35) > ... > > @@ +190,5 @@ > > > > +/* static */ bool > > +nsCocoaFeatures::IsAtLeastVersion(int32_t aMajor, int32_t aMinor, int32_t aBugFix) > > +{ > > + return OSXVersion() >= GetFullVersion(aMajor, aMinor, aBugFix); > > Did you mean `VersionAssemble`? I'll make the other changes, but to answer this one - in this case, I did mean GetFullVersion, as it's operating on the user values, so we want to do the <10.6 means 10.6, etc. modifications that function does.
Assignee | ||
Comment 37•10 years ago
|
||
(In reply to Jeff Gilbert [:jgilbert] from comment #35) > ... > > @@ +134,5 @@ > > /* static */ int32_t > > nsCocoaFeatures::OSXVersionMajor() > > { > > + MOZ_ASSERT((OSXVersion() & MAC_OS_X_VERSION_10_0_HEX) == MAC_OS_X_VERSION_10_0_HEX); > > + return 10; > > We have a function which does this. There's no reason to hard-code this. Just to clarify - no reason to hard-code returning 10? Which function? Best I can do here is: if (OSXVersion() & MAC_OS_X_VERSION_10_0_HEX) == MAC_OS_X_VERSION_10_0_HEX) { return 10; } else { MOZ_CRASH("we only handle 10"); } unless we now go ahead and implement "ready for major version non-10". We're using 0x1000 to capture "major version 10", so implementing non-10 is easy enough, I just didn't want to do it in this bug. Or did I misunderstand?
Assignee | ||
Comment 38•10 years ago
|
||
Attachment #8521556 -
Attachment is obsolete: true
Attachment #8523048 -
Flags: review+
Comment 39•10 years ago
|
||
(In reply to Milan Sreckovic [:milan] from comment #37) > (In reply to Jeff Gilbert [:jgilbert] from comment #35) > > ... > > > > @@ +134,5 @@ > > > /* static */ int32_t > > > nsCocoaFeatures::OSXVersionMajor() > > > { > > > + MOZ_ASSERT((OSXVersion() & MAC_OS_X_VERSION_10_0_HEX) == MAC_OS_X_VERSION_10_0_HEX); > > > + return 10; > > > > We have a function which does this. There's no reason to hard-code this. > > Just to clarify - no reason to hard-code returning 10? Which function? > Best I can do here is: > > if (OSXVersion() & MAC_OS_X_VERSION_10_0_HEX) == MAC_OS_X_VERSION_10_0_HEX) { > return 10; > } else { > MOZ_CRASH("we only handle 10"); > } > > unless we now go ahead and implement "ready for major version non-10". > We're using 0x1000 to capture "major version 10", so implementing non-10 is > easy enough, I just didn't want to do it in this bug. > > Or did I misunderstand? Don't leak this requirement into a getter. Just have the getter do its normal thing. Assert if you want to make it clear that it should be 10, but return based on the correct function anyways. I do think we should crash if we get the wrong value, but we should do that as soon as we can: We should crash when we originally create the version number.
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → milan
Assignee | ||
Comment 40•10 years ago
|
||
(In reply to Jeff Gilbert [:jgilbert] from comment #39) > (In reply to Milan Sreckovic [:milan] from comment #37) > > (In reply to Jeff Gilbert [:jgilbert] from comment #35) > > > ... > > > > > > @@ +134,5 @@ > > > > /* static */ int32_t > > > > nsCocoaFeatures::OSXVersionMajor() > > > > { > > > > + MOZ_ASSERT((OSXVersion() & MAC_OS_X_VERSION_10_0_HEX) == MAC_OS_X_VERSION_10_0_HEX); > > > > + return 10; > > > > > > We have a function which does this. There's no reason to hard-code this. > > > > Just to clarify - no reason to hard-code returning 10? Which function? > > Best I can do here is: > > > > if (OSXVersion() & MAC_OS_X_VERSION_10_0_HEX) == MAC_OS_X_VERSION_10_0_HEX) { > > return 10; > > } else { > > MOZ_CRASH("we only handle 10"); > > } > > > > unless we now go ahead and implement "ready for major version non-10". > > We're using 0x1000 to capture "major version 10", so implementing non-10 is > > easy enough, I just didn't want to do it in this bug. > > > > Or did I misunderstand? > > Don't leak this requirement into a getter. Just have the getter do its > normal thing. Assert if you want to make it clear that it should be 10, but > return based on the correct function anyways. > > I do think we should crash if we get the wrong value, but we should do that > as soon as we can: We should crash when we originally create the version > number. Understood. We actually don't have the correct function at this point, but I can put that together, at which point it will work for > 10 versions anyway. I could do it in a dependent bug, but doesn't seem worth it. So, given that 10 -> 0x1000, and assuming 11 -> 0x1100, 12 -> 0x1200, etc.: version = 0x1000 + (major-10) * 0x100 major = ((version & 0xFF00) - 0x1000) / 0x100 + 10 I'll capture those in a couple of functions and update the patch.
Comment 41•10 years ago
|
||
Oh, I see, yeah. 10.6 should really be 0xa60 not 0x1060. We're sanding with 8000-grit sandpaper at this point though. I would land the r+'d patch and do the rest in a follow-up. (I can do it, since I seem to care :P) Honestly, since we're at 10.10 already, I would actually recommend extra bits for the minor versions, and maybe also for bug-fix: 0x0a0603 for '10.6.3' ought to be enough for anybody. :)
Assignee | ||
Comment 42•10 years ago
|
||
Yeah, I started looking at this, and found that other files are defining the same magic hex values, so landing what we have so far is the way to go. See bug 1100530.
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 43•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/92e947245f1d
Keywords: checkin-needed
Comment 44•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/92e947245f1d
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Comment 45•9 years ago
|
||
(In reply to Milan Sreckovic [:milan] from comment #23) > Created attachment 8519160 [details] [diff] [review] > Allow depth texture on 10.8.3 and higher instead of holding out for 10.9 Wouldn't it be great if this stuff was in a data file and didn't require recompiling firefox.
Assignee | ||
Comment 46•9 years ago
|
||
It may be interesting to do in the context of all blacklisting and GfxInfo type changes (e.g., bug 668008)
You need to log in
before you can comment on or make changes to this bug.
Description
•