Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix for webgl-based canvas sources rendering flipped in 0.40.0 + non-animated optimization #5303

Merged
merged 4 commits into from
Sep 18, 2017

Conversation

lbud
Copy link
Contributor

@lbud lbud commented Sep 14, 2017

WebGL APIs and Canvas APIs are vertically flipped (gl reads from the lower left, canvas from the upper left), so I introduced #5300 in #5155. Eventually we should try to write render tests with a secondary webgl canvas 🤔 to catch bugs like this, but in the meantime this should make it into the 0.40.1 patch release.

Edit: this PR also modifies the intermediary read step to only reread if a canvas is supposed to be animated, if it hasn't been read yet, or if it is resized, to mitigate performance concerns for non-animated canvases (see #5301).

Fixes #5300.
Refs #5301.

Launch Checklist

  • briefly describe the changes in this PR
  • manually test the debug page

@lbud lbud changed the title Flip webgl-based canvas pixels before copying them to intermediary bu… Fix for webgl-based canvas sources rendering flipped in 0.40.0 Sep 14, 2017
imageData.data.set(data);
const flipped = new Uint8Array(this.width * this.height * 4);
for (let i = this.height - 1, j = 0; i >= 0; i--, j++) {
flipped.set(data.slice(i * this.width * 4, (i + 1) * this.width * 4), j * this.width * 4);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd suggest avoiding many slice operations since they create a new array every time. 4 lines setting each of the 4 values of the array will be faster.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The array looks like its an entire row, not just four values. It might make sense to try data.subarray(begin, end) which is like .slice() except it provides a view into the existing array without copying it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, you're right. In that case a secondary inner loop might get also work well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh cool, did not know about subarray — thanks @ansis @mourner. (I tried the secondary loop in this test as well for good measure: )
image

@lbud lbud changed the title Fix for webgl-based canvas sources rendering flipped in 0.40.0 Fix for webgl-based canvas sources rendering flipped in 0.40.0 + non-animated optimization Sep 15, 2017
@lbud lbud changed the title Fix for webgl-based canvas sources rendering flipped in 0.40.0 + non-animated optimization [not ready] Fix for webgl-based canvas sources rendering flipped in 0.40.0 + non-animated optimization Sep 15, 2017
@lbud lbud changed the title [not ready] Fix for webgl-based canvas sources rendering flipped in 0.40.0 + non-animated optimization Fix for webgl-based canvas sources rendering flipped in 0.40.0 + non-animated optimization Sep 15, 2017
for (let i = this.height - 1, j = 0; i >= 0; i--, j++) {
flipped.set(data.subarray(i * this.width * 4, (i + 1) * this.width * 4), j * this.width * 4);
}
this.canvasData.data.set(flipped);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One more thing — is it possible to do this.canvasData.data.set inside the loop instead of using a temporary flipped array?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants