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

Improving the projection of video by setting videoElement style #6038

Merged
merged 3 commits into from Jan 12, 2019
Merged

Improving the projection of video by setting videoElement style #6038

merged 3 commits into from Jan 12, 2019

Conversation

ronikar
Copy link
Contributor

@ronikar ronikar commented Feb 2, 2018

Video element by default saves on its ratio. So, when it is projected on the map, it still save on its ratio, and the projection not always precise. I changed the style of the video element in order to enable the element not to save on the ratio on the video.

I attached example on projection before my commit and after.

before commit
after commit

Copy link
Sponsor Collaborator

@cherniavskii cherniavskii left a comment

Choose a reason for hiding this comment

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

I'm wondering if it's suitable for every video 🤔
Maybe it should be configurable?

@ronikar
Copy link
Contributor Author

ronikar commented Feb 2, 2018

It depends on browser, not on video.
you can read more at https://www.w3schools.com/css/css3_object-fit.asp

@IvanSanchez
Copy link
Member

I'm looking at the compabitility table at https://developer.mozilla.org/en-US/docs/Web/CSS/object-fit , and it seems that IE/Edge cannot use this.

Is there any other solution for IE/Edge?

@IvanSanchez
Copy link
Member

hhmm, apparently one can apply scale transformations to video, but it seems a bit like a hack: https://stackoverflow.com/questions/3779771/html-5-video-stretch

@ronikar
Copy link
Contributor Author

ronikar commented Feb 2, 2018

Maybe I am wrong, because I don't use IE, but if I set attribute in style at js file that isn't supported, it's doesn't effect on style and dosen't throw exception. so , browsers that don't support this attribue will not be affected.

I don't want to implement new projection for video with scale transform, beacuse the implemention will complicate the code because there are some edge cases you need to take care.

@IvanSanchez , I have an idea how to improve the projection for IE/Edge. copy the code of videoOverlay and add this function inside:

_reset: function () { var image = this._image, bounds = new L.Bounds( this._map.latLngToLayerPoint(this._bounds.getNorthWest()), this._map.latLngToLayerPoint(this._bounds.getSouthEast())), size = bounds.getSize(); L.DomUtil.setPosition(image, bounds.min); var videoRatio = image.videoHeight / image.videoWidth; var boundRatio = size.y / size.x; image.style.transform = image.style.transform + "scale3d(1," + boundRatio / videoRatio + ", 1)"; image.style.width = size.x + 'px'; image.style.height = 'auto'; }

This solution has one disadvantage. You must to pass initialized videoElement (videoHeight and videoWidth need to be initialized) . In addition, I believe, you need to change the line that sets image.style.transform, and adapt it to your browser.

loop: true,

// @option saveAspectRatio: Boolean = true
// Whether the video will save aspect ratio after the projection.
Copy link
Member

Choose a reason for hiding this comment

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

Can you please change the name to keepAspectRatio here? Also adding a note about browser compatibility would be good.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

where need i to add a note about browser compatibility?

Copy link
Member

Choose a reason for hiding this comment

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

In these comments. Something like

// Whether the video will keep its original aspect ratio. Setting it to `false`
// only has effect on browsers which implement the 
// [`object-fit` CSS property](https://developer.mozilla.org/en-US/docs/Web/CSS/object-fit)".

@ronikar
Copy link
Contributor Author

ronikar commented Jan 12, 2019

@IvanSanchez @cherniavskii
Will this improvement be added to leaflet version soon?

@IvanSanchez IvanSanchez merged commit 62ee339 into Leaflet:master Jan 12, 2019
Schleuse pushed a commit to Schleuse/Leaflet that referenced this pull request Dec 3, 2019
…let#6038)

* Improving the projection of video by setting videoElement style

* use the improvement only for supported browsers and when user want it

* small fixes + add a note about browser compatibility
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.

None yet

3 participants