Skip to content

Fix dynamic video positioning - #41

Merged
magomez merged 1 commit into
WebPlatformForEmbedded:masterfrom
sarkanyi:master
Apr 19, 2016
Merged

Fix dynamic video positioning#41
magomez merged 1 commit into
WebPlatformForEmbedded:masterfrom
sarkanyi:master

Conversation

@sarkanyi

Copy link
Copy Markdown

The new position isn't applied correctly when video is repositioned dynamically using USE(HOLE_PUNCH_GSTREAMER) [possibly also with USE(HOLE_PUNCH_EXTERNAL) - not tested].
I have created a simple test file to show the problem. Please see attached zip.
Using the testpage, the initial position is wrong (too much to the left). When pressing "Key Down" on the keyboard it should set the video fullscreen, but the result is too much down. Pressing the key again sets the position back to the original.
testpage.zip

@eocanha

Copy link
Copy Markdown
Member

The patch looks fine, but I'd rest more assured if @magomez could have a look at it, since he's more acquainted with that part of the code.

@aazamansari

Copy link
Copy Markdown

Can we test this fix with following links:
http://youtubeonrepeat.com/
https://www.bing.com/?scope=video

@magomez

Copy link
Copy Markdown

@sarkanyi, could you please add the the check for HOLE_PUNCH_EXTERNAL so this works in that case as well?

@sarkanyi

Copy link
Copy Markdown
Author

http://youtubeonrepeat.com/ doesn't plays for me (possibly vp9? - I have to look into it later), however the patch breaks the bing player because for some reason in that one the absoluteContentBox() is at 0,0.
I will have to redo the calculations and fix the patch.
@magomez: Sure I can, but I can't test it so it's up to you guys to validate it.

@magomez

Copy link
Copy Markdown

Yeah, nothing is expected to play when using HOLE_PUNCH_EXTERNAL, as all the playback should be performed by an external player :)

I'll check the aboluteContentBox() issue for the external case and fix it, but please, add that check as it's needed for that use case to work. Thanks in advance :)

@magomez

Copy link
Copy Markdown

No need to. The external use case uses a different media player (MediaPlayerPrivateHolePunchBase) and needs to be adapted to the latest changes, but I'll take care of it.
Just adding the check is enough for now.

@sarkanyi

Copy link
Copy Markdown
Author

Fixed bing also, added HOLE_PUNCH_EXTERNAL and squashed.

@magomez
magomez merged commit e63742b into WebPlatformForEmbedded:masterApr 19, 2016
@sarkanyi

Copy link
Copy Markdown
Author

@magomez: I don't know about your side, but for me on https://www.youtube.com/watch?v=StZcUAPRRac the setPipeline() function is never called, thus the position and size is never updated with the changes (because the first time they are called the pipeline doesn't yet exists).
On my side I can fix it by adding an updateVideoRectangle() call in play() for example.
Should I create a pull request for this too?

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@sarkanyi@eocanha@aazamansari@magomez