Warn when drawing to a destination smaller than the viewport

RESOLVED FIXED in mozilla29

Status

()

--
enhancement
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: jgilbert, Assigned: jgilbert)

Tracking

unspecified
mozilla29
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

5 years ago
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.
(Assignee)

Comment 1

5 years ago
Deliberate use of this is unusual, but possible. Cases where this might be desired include rendering to tiles from a very large viewport.
Summary: Warn on RTT without updating the viewport → Warn when drawing to a destination smaller than the viewport
(Assignee)

Comment 2

5 years ago
Created attachment 824948 [details] [diff] [review]
warn-large-viewport
Assignee: nobody → jgilbert
Attachment #824948 - Flags: review?(bjacob)
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.
(Assignee)

Comment 4

5 years ago
(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 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?
Attachment #824948 - Flags: review?(bjacob) → review-
(Assignee)

Comment 6

5 years ago
Created attachment 825583 [details] [diff] [review]
warn-large-viewport

Alright, it only warns once per context.
Attachment #824948 - Attachment is obsolete: true
Attachment #825583 - Flags: review?(bjacob)
Attachment #825583 - Flags: review?(bjacob) → review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
(Assignee)

Updated

5 years ago
Blocks: 847714
(Assignee)

Comment 10

5 years ago
Created attachment 8357503 [details] [diff] [review]
warn-large-viewport
Attachment #8357503 - Flags: review?(bjacob)
(Assignee)

Comment 11

5 years ago
I needed to remove `inline` from a function to fix the build bustage. Thoughts?
(Assignee)

Updated

5 years ago
Attachment #825583 - Attachment is obsolete: true
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).
Attachment #8357503 - Flags: review?(bjacob) → review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/1da908db059b
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29

Comment 15

4 years ago
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.
You need to log in before you can comment on or make changes to this bug.