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

Add video quads #745

Merged
merged 8 commits into from
Nov 14, 2017
Merged

Add video quads #745

merged 8 commits into from
Nov 14, 2017

Conversation

manthey
Copy link
Contributor

@manthey manthey commented Nov 8, 2017

Video quads are currently only supported by the canvas renderer. If the video is not ready, a preview color can be shown, but not a preview image (to show a preview image, create an image quad at the same coordinates as the video quad).

  • Added color quads to the canvas renderer. This made things easier to test.
  • Improved some of the caching logic in quads to reduce reloading images and videos in some circumstances.
  • Added a utility method to invalidate one or all of the quad's caches. This removes using a private method in the quad example.
  • Added code to defer rendering if something is busy. This prevents flashing when seeking on a playing video.
  • Fixed an issue with the canvas transform on images and videos.
  • Update quadFeature documentation. Also update some renderer documentation.
  • Improve testing infrastructure. Don't test video in PhantomJS. Allow console.log messages to appear in tutorial test output.
  • Added a video to the quads example.
  • Add two video quad tutorials; one stand-alone quad and one quad positioned on a map.
  • Added tests.

Also update some renderer documentation.
Don't test video in PhantomJS.  Allow console.log messages to appear in tutorial test output.
Video quads are currently only supported by the canvas renderer.  If the video is not ready, a preview color can be shown, but not a preview image.

Added color quads to the canvas renderer.  This made things easier to test.

Improved some of the caching logic in quads to reduce reloading images and videos in some circumstances.

Added a utility method to invalidate one or all of the quad's caches.  This removes using a private method in the quad example.

Added code to defer rendering if something is busy.  This prevents flashing when seeking on a playing video.

Fixed an issue with the canvas transform on images and videos.
Add a video to the quads example.

Add two video quad tutorials; one stand-alone quad and one quad positioned on a map.
@manthey
Copy link
Contributor Author

manthey commented Nov 8, 2017

This PR is broken into separate commits to try to make it easier to review.

@@ -111,6 +111,13 @@ var canvasRenderer = function (arg) {
features = layer.features(),
i;

for (i = 0; i < features.length; i += 1) {
if (features[i]._delayRender()) {
Copy link
Member

Choose a reason for hiding this comment

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

should it be if (!features[i]._delayRender())? If delayRender is true which means you want to skip rendering or something I missed here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Calling render() reschedules the rendering to the next animation frame. The return then stops it from rendering right now.

Copy link
Member

Choose a reason for hiding this comment

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

ok, thanks perhaps adding a comment here might help but I am fine either way.

this._renderImageQuads = function (context2d, map) {
if (!m_quads.imgQuads.length) {
this._renderImageAndVideoQuads = function (context2d, map) {
if ((!m_quads.imgQuads || !m_quads.imgQuads.length) && (!m_quads.vidQuads || !m_quads.vidQuads.length)) {
Copy link
Member

Choose a reason for hiding this comment

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

Can we wrap this long line?

Update a minor spacing issue in the comments of the single video tutorial and update its thumbnail to match.
Copy link
Member

@aashish24 aashish24 left a comment

Choose a reason for hiding this comment

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

Looks great..just has 1-2 minor comments

return m_quads;
};

/**
* Initialize
* If the data has changed and caching has been used, one or all data items
Copy link
Member

Choose a reason for hiding this comment

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

this sentence seems incomplete or missing some words?

Copy link
Member

@aashish24 aashish24 left a comment

Choose a reason for hiding this comment

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

Nice work. thank you for addressing the comments.

LGTM

@manthey manthey merged commit 6c5c2e6 into master Nov 14, 2017
@manthey manthey deleted the video-quad branch November 14, 2017 14:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants