Closed Bug 908905 Opened 7 years ago Closed 7 years ago

Reduce blocking of packed_depth_stencil on mac+nv

Categories

(Core :: Canvas: WebGL, defect)

All
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla26

People

(Reporter: jgilbert, Assigned: jgilbert)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

In bug 814839, we hit a crash at least Mac 10.8.2 machines with at least one NV card in the WebGL conformance tests for depth_texture. To resolve it, we blocked packed_depth_stencil on NV+Mac. I believe we should instead be blocking depth_texture.

Also, running the test on my 10.8.3 machine with a GeForce 9400M works fine, so we should further investigate adding a blocklisting case for NV+Mac<=10.8.2.
Change it to depth_texture, switch to disabling the feature instead of the extension, and moved it to a place where we're already doing WordAroundDrivers.
Assignee: nobody → jgilbert
Attachment #794920 - Flags: review?(bjacob)
Blocks: 685184
Comment on attachment 794920 [details] [diff] [review]
unblock-depth-stencil2

Review of attachment 794920 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/gl/GLContext.cpp
@@ +433,5 @@
>          if (WorkAroundDriverBugs()) {
>              if (Renderer() == RendererAdrenoTM320) {
>                  MarkExtensionUnsupported(OES_standard_derivatives);
>              }
> +            

Trailing \w .

@@ +437,5 @@
> +            
> +#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

I feel that it is useful writing "bug $bug_id" rather than just "$bug_id" in comments.
Attachment #794920 - Flags: review?(bjacob) → review+
Comment on attachment 794920 [details] [diff] [review]
unblock-depth-stencil2

>From: Jeff Gilbert <jgilbert@mozilla.com>
>
>diff --git a/gfx/gl/GLContext.cpp b/gfx/gl/GLContext.cpp
>--- a/gfx/gl/GLContext.cpp
>+++ b/gfx/gl/GLContext.cpp
>@@ -429,16 +429,27 @@ GLContext::InitWithPrefix(const char *pr
> 
>         InitExtensions();
> 
>         // Disable extensions with partial or incorrect support.
>         if (WorkAroundDriverBugs()) {
>             if (Renderer() == RendererAdrenoTM320) {
>                 MarkExtensionUnsupported(OES_standard_derivatives);
>             }
>+
>+#ifdef XP_MACOSX
>+            // The Mac Nvidia driver, for versions up to and including 10.8, don't seem
>+            // to properly support this. (bug 814839)
>+            // This has been fixed in Mac OS X 10.9. (bug 907946)
>+            if (Vendor() == gl::GLContext::VendorNVIDIA &&
>+                !nsCocoaFeatures::OnMavericksOrLater())
>+            {
>+                MarkUnsupported(GLFeature::depth_texture);
>+            }
>+#endif
>         }
> 
>         NS_ASSERTION(!IsExtensionSupported(GLContext::ARB_pixel_buffer_object) ||
>                      (mSymbols.fMapBuffer && mSymbols.fUnmapBuffer),
>                      "ARB_pixel_buffer_object supported without glMapBuffer/UnmapBuffer being available!");
> 
>         if (SupportsRobustness()) {
>             if (IsExtensionSupported(ARB_robustness)) {
>@@ -907,28 +918,16 @@ GLContext::InitExtensions()
> 
>     if (WorkAroundDriverBugs() &&
>         Vendor() == VendorQualcomm) {
> 
>         // Some Adreno drivers do not report GL_OES_EGL_sync, but they really do support it.
>         MarkExtensionSupported(OES_EGL_sync);
>     }
> 
>-#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 (WorkAroundDriverBugs() &&
>-        Vendor() == gl::GLContext::VendorNVIDIA &&
>-        !nsCocoaFeatures::OnMavericksOrLater())
>-    {
>-        MarkExtensionUnsupported(gl::GLContext::EXT_packed_depth_stencil);
>-    }
>-#endif
>-
> #ifdef DEBUG
>     firstRun = false;
> #endif
> }
> 
> 
> // Take texture data in a given buffer and copy it into a larger buffer,
> // padding out the edge pixels for filtering if necessary
Cool, always wondered how 'edit as comment' worked.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/613a1dedd130
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Jeff, you mentioned investigating blacklisting for NV+Mac<=10.8.2 only, any reason that won't work? I really hate not having depth texture available to over half of our Mac users.
Blocks: 917628
(In reply to conor from comment #7)
> Jeff, you mentioned investigating blacklisting for NV+Mac<=10.8.2 only, any
> reason that won't work? I really hate not having depth texture available to
> over half of our Mac users.

Yep, that is now bug 918628.
(In reply to Jeff Gilbert [:jgilbert] from comment #8)
> (In reply to conor from comment #7)
> > Jeff, you mentioned investigating blacklisting for NV+Mac<=10.8.2 only, any
> > reason that won't work? I really hate not having depth texture available to
> > over half of our Mac users.
> 
> Yep, that is now bug 918628.

Hm, that's not the right number.  Is the bug still <= 10.8.2 (e.g. 10.8.5 should work if we didn't blacklist it?)
You need to log in before you can comment on or make changes to this bug.