Closed Bug 907946 Opened 6 years ago Closed 6 years ago

Re-enable EXT_packed_depth_stencil on OS X 10.9

Categories

(Core :: Graphics, defect)

All
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla26

People

(Reporter: guillaume.abadie, Assigned: guillaume.abadie)

References

Details

Attachments

(1 file, 1 obsolete file)

This extension has been blacklisted on OS X 10.9 running on a Nvidia driver. But the crash described in 814839 seams to be fixed, and the WebGL conformance test is passed.
Summary: Re-enable EXT_packed_depth_stencil os OS X 10.9 → Re-enable EXT_packed_depth_stencil on OS X 10.9
Attached patch patch revision 1 (obsolete) — Splinter Review
Attachment #793721 - Flags: review?(jgilbert)
Attachment #793721 - Flags: review?(bjacob)
Attached patch patch revision 2Splinter Review
I have forgotten to qrefresh before uploading... =D
Attachment #793721 - Attachment is obsolete: true
Attachment #793721 - Flags: review?(jgilbert)
Attachment #793721 - Flags: review?(bjacob)
Attachment #793723 - Flags: review?(jgilbert)
Attachment #793723 - Flags: review?(bjacob)
Comment on attachment 793723 [details] [diff] [review]
patch revision 2

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

::: gfx/gl/GLContext.cpp
@@ +830,3 @@
>      if (WorkAroundDriverBugs() &&
> +        Vendor() == gl::GLContext::VendorNVIDIA &&
> +        !nsCocoaFeatures::OnMavericksOrLater())

We should be doing this with our blacklisting system, not hard-coded. We're not trying to use a 10.9 feature, but rather stop ourselves from using apparently broken drivers from 10.8 and earlier.
Attachment #793723 - Flags: review?(jgilbert) → review-
Oh I see... So, why is that been done this way?
Comment on attachment 793723 [details] [diff] [review]
patch revision 2

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

In theory, I agree with Jeff: it would be nice to have all these things handled by the blacklisting system.

The problem is that we do not currently have in our blocklisting system a Feature for "EXT_packed_depth_stencil", so we would have to add one, that that is very dangerous with our stupid blocklisting system: if a new downloadable blacklist entry were added with that feature, it would get mis-interpreted as "all features" by older Gecko's. That has severely bitten us before.

So for now --- pending the blocklisting system rewrite that has been discussed and even designed but never prioritized --- we will have to do with hardcoded checks as in the present patch.

Jeff: please consider lifting the r-
Guillaume: please do not land as long as there is a r-
Attachment #793723 - Flags: review?(bjacob) → review+
For sure! Thanks!
(In reply to Benoit Jacob [:bjacob] from comment #5)
> Comment on attachment 793723 [details] [diff] [review]
> patch revision 2
> 
> Review of attachment 793723 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> In theory, I agree with Jeff: it would be nice to have all these things
> handled by the blacklisting system.
> 
> The problem is that we do not currently have in our blocklisting system a
> Feature for "EXT_packed_depth_stencil", so we would have to add one, that
> that is very dangerous with our stupid blocklisting system: if a new
> downloadable blacklist entry were added with that feature, it would get
> mis-interpreted as "all features" by older Gecko's. That has severely bitten
> us before.
> 
> So for now --- pending the blocklisting system rewrite that has been
> discussed and even designed but never prioritized --- we will have to do
> with hardcoded checks as in the present patch.
> 
> Jeff: please consider lifting the r-
> Guillaume: please do not land as long as there is a r-
Alright, that was my fear. Let's just hardcode this then.
Attachment #793723 - Flags: review- → review+
https://hg.mozilla.org/mozilla-central/rev/13bbb02033f8
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
You need to log in before you can comment on or make changes to this bug.