933009 - Warn when drawing to a destination smaller than the viewport

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, enhancement)

Description

[Kelsey Gilbert [:jgilbert]]

I am embarrassed to say that I wasted an hour yesterday tracking this issue down in a test script I'm writing. It seems like an obvious thing to warn on, with fairly high correlation to errors.

More exactly, we should warn when a drawArrays/drawElements call is made against a non-null framebuffer, and 'viewport has not been set accordingly'.

I think the easiest way to help is to generally warn when the current viewport is larger than the FB we're rendering to.

Comment 1

[Kelsey Gilbert [:jgilbert]]

Deliberate use of this is unusual, but possible. Cases where this might be desired include rendering to tiles from a very large viewport.

Comment 2

[Kelsey Gilbert [:jgilbert]]

Attachment: warn-large-viewport

Comment 3

[u480271]

Drive-by review: Looks good to me.

But really to provide feedback what you describe in the description, we should flag when a RTT is set and only clear the flag once a viewport is set. If the flag is set when hitting a draw call, emit a warning.

Comment 4

[Kelsey Gilbert [:jgilbert]]

(In reply to Dan Glastonbury :djg :kamidphish from comment #3)
> Drive-by review: Looks good to me.
> 
> But really to provide feedback what you describe in the description, we
> should flag when a RTT is set and only clear the flag once a viewport is
> set. If the flag is set when hitting a draw call, emit a warning.

The issue here is that they could set the viewport *before* binding a rendertarget. (stateful APIs \o/)

Comment 5

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 824948 [details] [diff] [review]
warn-large-viewport

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

We might need a second round if you agree with this:

::: content/canvas/src/WebGLContextVertices.cpp
@@ +741,5 @@
> +            mViewportHeight > rect->Height())
> +        {
> +            GenerateWarning("Drawing to a destination rect smaller than the viewport rect.");
> +        }
> +    }

This warning looks OK-ish, but as you mention, there are valid use cases where it will be generated. Another concern is that even when this warning is useful, being generated in every draw call, it will likely max out the max-warning-per-context limit immediately. Could you make it a one-off warning at least?

Comment 6

[Kelsey Gilbert [:jgilbert]]

Attachment: warn-large-viewport

Alright, it only warns once per context.

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f35e995c0f4d

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Backed out for OSX bustage.
https://hg.mozilla.org/integration/mozilla-inbound/rev/0fb5a87da513

https://tbpl.mozilla.org/php/getParsedLog.php?id=29988416&tree=Mozilla-Inbound

Comment 9

[Kelsey Gilbert [:jgilbert]]

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

Comment 10

[Kelsey Gilbert [:jgilbert]]

Attachment: warn-large-viewport

Comment 11

[Kelsey Gilbert [:jgilbert]]

I needed to remove `inline` from a function to fix the build bustage. Thoughts?

Comment 12

[Benoit Jacob [:bjacob] (mostly away)]

Yes, I think what happened is that a function defined 'inline' in one translation unit that does not call it may be omitted (the assumption being that if another translation unit needs to call this inline function, it will take care of definining it; to that effect, 'inline' allows multiple definitions).

Comment 13

[Ryan VanderMeulen [:RyanVM]]

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

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1da908db059b

Comment 15

[Florian Bösch]

This issue is not resolved. The warning is still displayed, and it's a perfectly legitimate usecase to use a viewport smaller than the attached framebuffer (one such usecase is using it to implement mouse picking).

Please remove the warning, because it's annoying.