867460 - remove notion of ShaderProgramTypes from gfx/gl

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Andreas Gal :gal]

We want to move towards a dynamic shader pipeline, so having ShaderProgramTypes in gfx/gl is not useful.

Comment 1

[Andreas Gal :gal]

Attachment: patch, works on macosx

Comment 2

[Andreas Gal :gal]

Attachment: and the right patch this time

Comment 3

[Andreas Gal :gal]

review ping

Comment 4

[Bas Schouten (:bas.schouten)]

I'll get to this by thursday.

Comment 5

[Andreas Gal :gal]

review ping

Comment 6

[Bas Schouten (:bas.schouten)]

Comment on attachment 743966 [details] [diff] [review]
and the right patch this time

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

Sorry for the delay.

This is strictly an improvement in my opinion. There seem to be a lot of whitespace issues in this code but I don't think now is the right time to address them, we should though, sometime.

Comment 7

[Andreas Gal :gal]

Attachment: rebased

This was complete hell to rebase. We have to accelerate reviews of patches to avoid this.

Comment 8

[Andreas Gal :gal]

Milan, can you find someone to land this? I probably won't have time to watch the tree in case this bounces. This is blocking any progress on CSS filters, so it would be good to get this in. I tested this on mac (works). Thanks.

Comment 9

[Andreas Gal :gal]

Attachment: two small fixes for android and linux

Comment 10

[Andreas Gal :gal]

https://tbpl.mozilla.org/?tree=Try&rev=a184cc99fa0d

Comment 11

[Andreas Gal :gal]

Attachment: another fix for B2G

Kicked off another try run: https://tbpl.mozilla.org/?tree=Try&rev=08cb52371c17

This is probably ready to land.

Comment 12

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to Andreas Gal :gal from comment #8)
> Milan, can you find someone to land this? I probably won't have time to
> watch the tree in case this bounces. This is blocking any progress on CSS
> filters, so it would be good to get this in. I tested this on mac (works).
> Thanks.

Will do.  Waiting for the try run in Comment 11 to finish.

Comment 13

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to Andreas Gal :gal from comment #11)
> Created attachment 766649 [details] [diff] [review]
> another fix for B2G
> 
> Kicked off another try run:
> https://tbpl.mozilla.org/?tree=Try&rev=08cb52371c17
> 
> This is probably ready to land.

One more B2G issue left, pushed another to try: https://tbpl.mozilla.org/?tree=Try&rev=33480e49cd98

Comment 14

[Andreas Gal :gal]

Attachment: fix another b2g issue

Comment 15

[Andreas Gal :gal]

another try run for the b2g fix: https://tbpl.mozilla.org/?tree=Try&rev=712a99df82b2

Comment 16

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Andreas' patch with the last B2G compile error fixed

Carrying Bas' r+ from attachment 743966 [details] [diff] [review].

Comment 17

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to Milan Sreckovic [:milan] from comment #16)
> Created attachment 766828 [details] [diff] [review]
> Andreas' patch with the last B2G compile error fixed
> 
> Carrying Bas' r+ from attachment 743966 [details] [diff] [review].

Try run for this: https://tbpl.mozilla.org/?tree=Try&rev=33480e49cd98

Comment 18

[Joe Drew (not getting mail)]

Comment on attachment 766828 [details] [diff] [review]
Andreas' patch with the last B2G compile error fixed

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

I do very much like this. I'd like it even more if ShaderProgramType was removed entirely instead of just moved to LayerManagerOGLProgram.h, but that's probably a task for bug 886315. Consider this my intent-approval!

Note for committing: Make sure the user is set to Andreas and not Milan!

Comment 19

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Andreas' patch with the last B2G compile error fixed

Carrying the r+ from Bas; superreview from Joe for the intent.

Comment 20

[Joe Drew (not getting mail)]

Comment on attachment 767221 [details] [diff] [review]
Andreas' patch with the last B2G compile error fixed

Still agree with it as in comment 18 :)

Comment 21

[Andreas Gal :gal]

Correct, I have already deleted ShaderProgramType in my local repo. Patch this week hopefully.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1bf0cdeacb22

Comment 23

[(no longer active)]

Baked out for assertions in various test suites:

https://hg.mozilla.org/integration/mozilla-inbound/rev/9f43af909f29

(See <https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=da5456a94c1b> and <https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=06bc878dd47f>)

Comment 24

[Phil Ringnalda (:philor)]

Now that we have some hindsight to see exactly what this broke, the tally is:

* assertions on OS X 10.6 only, in mochitest-1, -3, -5, -other (-chrome and -a11y), and reftests and crashtests

* failures in every plugin reftest on opt and debug OS X 10.6, 10.7 and 10.8

* two unexpected passes in b2g reftest-6, in webgl-color-alpha-test.html

Comment 25

[Milan Sreckovic [:milan] (needinfo for best results)]

Thanks for backing this out, and sorry for the bother.  We'll sort it out.

Comment 26

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 766781 [details] [diff] [review]
fix another b2g issue

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

This is the actual patch, so I was commenting on it, even though it's tagged obsolete for different reasons.  I just wanted to check if we're losing some information, mostly on the 565 format, so if you have a chance, check the comments.

::: gfx/gl/GLContext.cpp
@@ +1462,5 @@
>      if (currentPackAlignment != 4) {
>          fPixelStorei(LOCAL_GL_PACK_ALIGNMENT, currentPackAlignment);
>      }
>  
> +    if (aFormat == FORMAT_R8G8B8A8 || aFormat == FORMAT_R8G8B8X8) {

Here we used to catch RGBALayerProgramType and RGBXLayerProgramType, and now we just catch the 8-bit per channel versions.  Is that OK?

@@ +2086,5 @@
>              break;
>          case gfxASurface::ImageFormatRGB16_565:
>              format = LOCAL_GL_RGB;
>              type = LOCAL_GL_UNSIGNED_SHORT_5_6_5;
> +            surfaceFormat = FORMAT_R5G6B5;

See the comment at 1466 - this format used to go into RGBALayerProgramType, so the test at 1466 would catch it, and we would swap R and B.  With this change, we won't have that swap for 565.  Is that OK?

Also, ImageFormatRGB16_565 used to go to RGBALayerProgamType, but now goes to RGBXLayerProgramType - is that OK?

::: gfx/gl/GLContextProviderCGL.mm
@@ +200,5 @@
>                                          SharedTextureHandle sharedHandle,
>                                          SharedHandleDetails& details)
>      {
>          details.mTarget = LOCAL_GL_TEXTURE_RECTANGLE_ARB;
> +        details.mTextureFormat = FORMAT_R8G8B8A8;

With this change, we will hit the R-B swap in GLContext.cpp on this format, where before we did not (because we weren't doing it for RGBRectLayerProgramType.)  Is this OK?

::: gfx/gl/GLContextProviderEGL.cpp
@@ +981,5 @@
>      case SharedHandleType::SurfaceTexture: {
>          SurfaceTextureWrapper* surfaceWrapper = reinterpret_cast<SurfaceTextureWrapper*>(wrapper);
>  
>          details.mTarget = LOCAL_GL_TEXTURE_EXTERNAL;
> +        details.mTextureFormat = FORMAT_R8G8B8A8;

Same thing - this will now do a R-B swap, before it didn't.  Good?

Comment 27

[Milan Sreckovic [:milan] (needinfo for best results)]

Lost somewhere the comment about LayerManagerOGLProgram.h - 565 used to go to RGBALayer..., now it goes to RGBXLayer... - this change is OK?

Comment 28

[Andreas Gal :gal]

> > +    if (aFormat == FORMAT_R8G8B8A8 || aFormat == FORMAT_R8G8B8X8) {
> 
> Here we used to catch RGBALayerProgramType and RGBXLayerProgramType, and now
> we just catch the 8-bit per channel versions.  Is that OK?

Swapping the channels only works on 8-bit-per-color, so this should be ok.

> 
> See the comment at 1466 - this format used to go into RGBALayerProgramType,
> so the test at 1466 would catch it, and we would swap R and B.  With this
> change, we won't have that swap for 565.  Is that OK?

Again, I think RGB16 has no swap. But we will need a 2nd pair of eyes on here.

> 
> Also, ImageFormatRGB16_565 used to go to RGBALayerProgamType, but now goes
> to RGBXLayerProgramType - is that OK?

RGB has no alpha so I am not sure why this worked before.

> >      {
> >          details.mTarget = LOCAL_GL_TEXTURE_RECTANGLE_ARB;
> > +        details.mTextureFormat = FORMAT_R8G8B8A8;

This seems like a bug!

> >  
> >          details.mTarget = LOCAL_GL_TEXTURE_EXTERNAL;
> > +        details.mTextureFormat = FORMAT_R8G8B8A8;
> 
> Same thing - this will now do a R-B swap, before it didn't.  Good?

This too.

Comment 29

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to Andreas Gal :gal from comment #28)
...
> > See the comment at 1466 - this format used to go into RGBALayerProgramType,
> > so the test at 1466 would catch it, and we would swap R and B.  With this
> > change, we won't have that swap for 565.  Is that OK?
> 
> Again, I think RGB16 has no swap. But we will need a 2nd pair of eyes on
> here.

Second set of eyes (bjacob) confirms this.

> 
> > >      {
> > >          details.mTarget = LOCAL_GL_TEXTURE_RECTANGLE_ARB;
> > > +        details.mTextureFormat = FORMAT_R8G8B8A8;
> 
> This seems like a bug!
> 
> > >  
> > >          details.mTarget = LOCAL_GL_TEXTURE_EXTERNAL;
> > > +        details.mTextureFormat = FORMAT_R8G8B8A8;
> > 
> > Same thing - this will now do a R-B swap, before it didn't.  Good?
> 
> This too.

Will sort this out with BenWa.

Comment 30

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 767221 [details] [diff] [review]
Andreas' patch with the last B2G compile error fixed

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

::: gfx/gl/GLContextProviderCGL.mm
@@ +200,5 @@
>                                          SharedTextureHandle sharedHandle,
>                                          SharedHandleDetails& details)
>      {
>          details.mTarget = LOCAL_GL_TEXTURE_RECTANGLE_ARB;
> +        details.mTextureFormat = FORMAT_R8G8B8A8;

This seems to be a problem.  We lose the extra information, and infer RGBALayerProgramType from the format and YouTube trailers break. Likely the other plug-ins as well.

Comment 31

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Modified patch to pass try

Use the surface format to figure out the program type, except in a couple of places where the extra target information is necessary to give us the external or rect program instead.
This is try run https://tbpl.mozilla.org/?tree=Try&rev=804b875443d3.  It has an unexpected pass on ARM B2G, webgl color alpha test. There is a place where we previously sent 565 to RGBA, and now to RGBX - would that explain the previous fail?

Comment 32

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: For the reviewers - capture the difference from the obsolete patch.

This is not a patch to be applied, but collects the differences from the patch Andreas submitted to perhaps make the review easier.

Comment 33

[Kelsey Gilbert [:jgilbert]]

(In reply to Milan Sreckovic [:milan] from comment #31)
> Created attachment 769487 [details] [diff] [review]
> Modified patch to pass try
> 
> Use the surface format to figure out the program type, except in a couple of
> places where the extra target information is necessary to give us the
> external or rect program instead.
> This is try run https://tbpl.mozilla.org/?tree=Try&rev=804b875443d3.  It has
> an unexpected pass on ARM B2G, webgl color alpha test. There is a place
> where we previously sent 565 to RGBA, and now to RGBX - would that explain
> the previous fail?

Yes, this could cause this if we just weren't fuzzing the values correctly before.

Comment 34

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to Jeff Gilbert [:jgilbert] from comment #33)
> (In reply to Milan Sreckovic [:milan] from comment #31)
> > Created attachment 769487 [details] [diff] [review]
> > Modified patch to pass try
> > 
> > Use the surface format to figure out the program type, except in a couple of
> > places where the extra target information is necessary to give us the
> > external or rect program instead.
> > This is try run https://tbpl.mozilla.org/?tree=Try&rev=804b875443d3.  It has
> > an unexpected pass on ARM B2G, webgl color alpha test. There is a place
> > where we previously sent 565 to RGBA, and now to RGBX - would that explain
> > the previous fail?
> 
> Yes, this could cause this if we just weren't fuzzing the values correctly
> before.

Thanks Jeff. I'll see if I can narrow it down, but it looks like we tagged these as failing with the streaming buffers, they ran before, so there shouldn't be too much opposition to them running again :)

Comment 35

[Kelsey Gilbert [:jgilbert]]

Where possible, we should see if we can add a fuzz window instead of just marking it failing. A test *actually* failing is something which *needs* to be fixed, not ignored. (The only tests we should consider disabling are the ones for our non-default prefs, and even then, only with good reason)

Comment 36

[Milan Sreckovic [:milan] (needinfo for best results)]

So, we would open a separate bug (is there one?) to get these tests back to passing, potentially with fuzzing, then when this change keeps the tests passing, we're in good shape.  I like that.

Comment 37

[Kelsey Gilbert [:jgilbert]]

(In reply to Milan Sreckovic [:milan] from comment #36)
> So, we would open a separate bug (is there one?) to get these tests back to
> passing, potentially with fuzzing, then when this change keeps the tests
> passing, we're in good shape.  I like that.

If they were already disabled, yes. If they start failing from new code, they strongly indicate that something is wrong, and should be fixed before landing the new code.

Comment 38

[Milan Sreckovic [:milan] (needinfo for best results)]

They got fuzzed in the streaming buffers patch for bug 716859, then disabled in bug 868892 because bug 868556 started the consistent failure, and that it's somehow related to bug 869011. It's a tangled web.

These started not failing from the new code, but let me find out what makes them pass.

Comment 39

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Modified patch to pass try

The reftest that started inexplicably failing with bug 868556 (tracked in bug 868892) are passing again.

Comment 40

[Benoit Girard (:BenWa)]

Comment on attachment 769491 [details] [diff] [review]
For the reviewers - capture the difference from the obsolete patch.

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

::: gfx/layers/opengl/LayerManagerOGLProgram.h
@@ +73,5 @@
> +                                 gfx::SurfaceFormat aFormat)
> +{
> +  switch(aTarget) {
> +    case LOCAL_GL_TEXTURE_EXTERNAL:
> +      return RGBALayerExternalProgramType;

optional: We could throw in some asserts that the format matches RGBA for extra safety.

Comment 41

[Benoit Girard (:BenWa)]

Comment on attachment 771416 [details] [diff] [review]
Modified patch to pass try

I looked at the changes to handle rect texture and it looks good.

Comment 42

[Matt Woodrow (:mattwoodrow)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/55258de26ac3

Comment 43

[Andreas Gal :gal]

Thats for fixing up my mess here. I owe various people a beer at the next gfx work week.

Comment 44

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/55258de26ac3

Comment 45

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to Benoit Girard (:BenWa) from comment #40)
> 
> ::: gfx/layers/opengl/LayerManagerOGLProgram.h
> @@ +73,5 @@
> > +                                 gfx::SurfaceFormat aFormat)
> > +{
> > +  switch(aTarget) {
> > +    case LOCAL_GL_TEXTURE_EXTERNAL:
> > +      return RGBALayerExternalProgramType;
> 
> optional: We could throw in some asserts that the format matches RGBA for
> extra safety.

Filed as bug 890929.