More fixes for specific sample apk. #1166
Conversation
Do a little refactoring while I’m there.
Errors are still reported with full severity in the report view, but this: a) Let’s you see something in the views (much like most Android devices) b) Doesn’t mean compilation errors snowball into other report issues.
| @@ -601,12 +601,20 @@ func compat(ctx context.Context, device *device.Instance) (transform.Transformer | |||
| return | |||
| } | |||
| case *GlTexStorage2DMultisample: | |||
| { | |||
| if version.IsES || version.AtLeastGL(4, 3) { | |||
There was a problem hiding this comment.
I would drop the 4.3 part of the condition. But that is probably just me.
There was a problem hiding this comment.
I'd like to steer the compat code towards doing the right checks. My longer term plan of action is to pull this logic out of this mega function and as methods of the command. We can then use these methods check to see if the particular command is supported by the given device - and use this to pick the most suitable device and/or show compatibility warnings in the future.
There was a problem hiding this comment.
I agree with regard to checks - but in different way - ideally add some support for desktop GL into the api files as well, and then mutate the output through that to verify it should work. For a set of devices this would tell us which would fail (and how), and which would pass, all without trying to run it on the devices. As a poor man's version of this you could just record the minimal requirements as you execute the compat. So even without any replay device you would get transformed stream and the needed min spec.
With regards transforms - I prefer unconditional ones. That is, simplify complicated GL commands to simpler canonical ones. This is great example of that. There is no reason not to always simplify to more supported glTexImage2DMultisample version. Similarly all other *Storage methods can be simplified to more supported GL commands (although that is a bit more code, and we did not hit that compat need yet).
Anyway, it just my 1 cent. Feel free to follow your path. But if you get to situation where you can not reproduce a bug because the user's GL version and set of extensions follows a different path then yours, don't say I didn't warn you :-P (e.g. if the AtLeast(4,3) part of this block gets accidentally broken in the future as part of refactoring, you will never find out on your machine)
gapis/api/gles/compat.go
Outdated
| } else { | ||
| // glTexStorage2DMultisample is not supported by replay device. | ||
| // Drop down to a non-multisampled version. | ||
| // TODO: Show report warning. |
There was a problem hiding this comment.
Drop last two comments.
It is not downgrading to signle-sample.
I do not see a need to warn. It is perfectly valid compat.
Spec: "Calling TexStorage2DMultisample is
equivalent to calling TexImage2DMultisample with the equivalently named parameters
set to the same values, except that the resulting texture has immutable
format."
The immutable part is irrelevant for compat.
There was a problem hiding this comment.
You are correct - these comments were for a different fallback command I was trying. Then I noticed glTexImage2DMultisample existed in an earlier profile, so switched to using that.
Fixed the comments.
| // It may not be implemented by the replay driver. | ||
| // It is only a hint so we can just drop it. | ||
| // TODO: It has performance impact so we should not ignore it when profiling. | ||
| return | ||
|
|
||
| case *GlInvalidateFramebuffer, *GlInvalidateSubFramebuffer: | ||
| if !version.AtLeastES(3, 0) || !version.AtLeastGL(4, 3) { |
There was a problem hiding this comment.
Again, I would prefer unconditional. You might disagree.
| std::vector<unsigned int> spirv; | ||
|
|
||
| EShMessages messages = EShMsgDefault; | ||
| EShMessages messages = relaxed_errs ? EShMsgRelaxedErrors : EShMsgDefault; |
There was a problem hiding this comment.
Oh, glslang has native support for relaxed compilation?
That makes this less of a hack. Good!
There was a problem hiding this comment.
Yeah - I spent a while trying to hack away, then found this. Happy days.
It doesn’t exist on mac (needs 4.3, got 4.1) Use glTexImage2DMultisample instead.
Don’t remove glInvalidateFramebuffer if it is supported.
No description provided.