Closed Bug 790712 Opened 8 years ago Closed 7 years ago

Implement WEBGL_compressed_texture_atc

Categories

(Core :: Canvas: WebGL, defect, critical)

defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla18
blocking-kilimanjaro ?
blocking-basecamp +

People

(Reporter: bjacob, Assigned: bjacob)

References

(Blocks 1 open bug)

Details

(Whiteboard: webgl-extension webgl-next [games:p1] [LOE:S])

Attachments

(1 file, 2 obsolete files)

Attached patch atc support (obsolete) — Splinter Review
This patch hides this feature behind a preference, which is set on B2G only. So in principle, if needed, we can land it, as it's not actually enabled on any shipping product, but it's best if we can wait to first get the extension promoted to draft status. Working on it --- writing a conformance test. We'll see what to do by next week.
Severity: normal → critical
OS: Linux → All
Hardware: x86_64 → All
Depends on: 790723
While this would be nice to land, it's not a v1 blocker.  Re-nom if you disagree :)
blocking-basecamp: ? → -
Renomming -- It's essential for WebGL performance on the first target B2G devices (ATC is the only texture compression they will support)...
blocking-basecamp: - → ?
I spoke with Vlad about this and it's quite important, plus the work is almost ready to go (hence the LOE:S).  We can always re-evaluate closer to the release on the off chance this didn't make it in by then.
Assignee: nobody → bjacob
blocking-basecamp: ? → +
Whiteboard: webgl-extension webgl-next [games:p1] → webgl-extension webgl-next [games:p1] [LOE:S]
Just to add a data point to this, the EA game developers on the WebGL thread are pretty specific that, with memory and performance limitations on mobile being what they are, compressed textures are essential to get anything but the most basic 2d or 3d game to work.
Attached patch atc support (obsolete) — Splinter Review
Updated: fixes an assert failure in DEBUG builds.
Attachment #660537 - Attachment is obsolete: true
Just to be clear --- this is all in the realm of Khronos now, right?  No more assistance needed from our friends?
Correct: the WebGL WG at Khronos will decide whether this can move to draft status, and we can help there by writing a nice conformance test, which I'm working on. No more help needed from any specific company. If we don't get this moved to draft status early enough, we will still have the option of landing this regardless, relying on this being preffed off on currently shipping products (Firefox), but that would be sub-optimal.
Depends on: 791432
Promoted to draft status:
http://www.khronos.org/registry/webgl/extensions/WEBGL_compressed_texture_atc/

Can implement now.
Summary: Implement WEBGL_compressed_texture_atc (can't land until the spec proposal is promoted to draft) → Implement WEBGL_compressed_texture_atc
Attachment #661367 - Flags: review?(jgilbert)
Attached patch atc supportSplinter Review
Attachment #661367 - Attachment is obsolete: true
Attachment #661367 - Flags: review?(jgilbert)
Attachment #663852 - Flags: review?(jgilbert)
Comment on attachment 663852 [details] [diff] [review]
atc support

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

A nit about vendor suffixes in our internal stuff. Non-compulsory, since we have tons of suffixed constants in there, but I'd like to move us away from that.

::: content/canvas/src/WebGLContextValidate.cpp
@@ +416,3 @@
>              }
> +            ErrorInvalidOperation("%s: level parameter does not match width and height", info);
> +            return false;

Interesting use of switch and break for control flow. I like it.

::: gfx/gl/GLDefs.h
@@ +3093,5 @@
>  #define LOCAL_WGL_CONTEXT_COMPATIBILITY_PROFILE_BIT_ARB     0x00000002
>  #define LOCAL_WGL_CONTEXT_ROBUST_ACCESS_BIT_ARB             0x00000004
>  
> +// AMD_compressed_ATC_texture
> +#define LOCAL_GL_ATC_RGB_AMD                                0x8C92

I would prefer these without the vendor suffix. (Unless there is a conflict which would require using suffixes) It's fine to leave it like this, but these suffixes are really just unnecessary namespacing noise.
Attachment #663852 - Flags: review?(jgilbert) → review+
https://hg.mozilla.org/mozilla-central/rev/c066bfd5d4e8
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.