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

Alps 904 - Video quality change #73

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

aberthet-gpsw
Copy link
Contributor

When requesting quickly a change in the quality of a video, the HTMLVideoElement wasn't always ready and thus null. We tried to remove some EventListener of something null, so it crashed. It is not the case anymore, as a check on the HTMLVideoElement (either null or undefined) is done.

Copy link
Contributor

@rroux-gpsw rroux-gpsw left a comment

Choose a reason for hiding this comment

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

It is not ok for me ! Sometimes the video restart when changing quality. Sometimes we lost the current time display, and I even got a black screen with only the sound ... I don't know if it's related to this PR, I'll push my tests further.

@aberthet-gpsw
Copy link
Contributor Author

aberthet-gpsw commented May 16, 2017

Not related, but I can take a look at it and push it inside this PR. Note that there are also problems with the VideoControls plugin.

@rroux-gpsw
Copy link
Contributor

You're right @aberthet-gpsw I can reproduce the black screen issue on my mac/chrome with the master branch. I don't remember that happened on my Unbuntu desktop I will have to test again on Ubuntu to have a clear view of the bug. Have to test Firefox too !

@rroux-gpsw
Copy link
Contributor

@aberthet-gpsw Please if you want to have a look on this issue, come see me if you can't reproduce. 😁

HTMLVideoElement wasn't always ready and thus null. We tried to remove
some EventListener of something null, so it crashed. It is not the case
anymore, as a check on the HTMLVideoElement (either null or undefined)
is done.
@aberthet-gpsw aberthet-gpsw force-pushed the alps-904-video-quality-change branch from 14c7c94 to be6939f Compare May 29, 2017 13:53
@sw-team-release-gpsw
Copy link

sw-team-release-gpsw commented May 29, 2017

CLA assistant check
All committers have signed the CLA.

- first one was when you made a quick request in the quality change of
the video, the video would crash
- second one was when you made a quick request in the quality change of
the video, when reaching the end the video would crash, even if resuming
was set to true
@aberthet-gpsw
Copy link
Contributor Author

So I'm stopping here my changes, as I can't go further. The blocking element here is the loadstart event of the HTMLVideoElement which is fired not fast enough. If we change the quality of the video really quickly, before this event is fired, we get the bug we know, and I can't find a solution to fix that.

I propose to let it as it is, given that this bug shouldn't be reproduced by the vast majority of users. And as we plan to change in the future the inner workings of playable object (Video, Sound etc...), we may fix it at this moment.

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