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)

33 Branch
x86_64
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: nick, Assigned: milan)

References

Details

(Whiteboard: webgl-site)

Attachments

(3 files, 4 obsolete files)

Attached image FF Release
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/
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
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
Did this ever work, is it a recent regression?
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.
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
On the off chance that I don't know what "UE4 strategy game demo" is, can we get some details?
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.
Attached image FF Nightly
Attachment #8517591 - Attachment description: Screen Shot 2014-11-05 at 10.32.06 AM.png → FF Release
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?
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]
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.
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)
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.
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
(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.
Whiteboard: webgl-site
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.
Component: Desktop → Graphics
Product: Tech Evangelism → Core
Version: Firefox 33 → 33 Branch
(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...
Never mind.  Friday afternoon.  Duh...
Nick, can you try this patch?
Flags: needinfo?(nick)
Attachment #8519160 - Flags: review?(vladimir)
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 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-
Flags: needinfo?(nick)
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.
Existing code doesn't really work for major version > 10, not that we have cases for that...
(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.
(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;
Attachment #8520367 - Flags: review?(jgilbert) → review+
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)
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+
(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.
(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?
(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: nobody → milan
(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.
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. :)
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.
https://hg.mozilla.org/mozilla-central/rev/92e947245f1d
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
(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.
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.