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(mpe): show player when transport controls are disabled #15196

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

Conversation

ramezgerges
Copy link
Contributor

GitHub Issue (If applicable): #14735

PR Type

What kind of change does this PR introduce?

What is the current behavior?

What is the new behavior?

PR Checklist

Please check if your PR fulfills the following requirements:

Other information

Internal Issue (If applicable):

@unodevops
Copy link
Contributor

🤖 Your Docs stage site is ready! Visit it here: https://unodocsprstaging.z13.web.core.windows.net/pr-15196/index.html

@ramezgerges
Copy link
Contributor Author

When_MediaPlayerElement_SetIsFullWindow_Check_Fullscreen seems to be failing in CI only, but the remaining tests are finally passing.

@unodevops
Copy link
Contributor

🤖 Your Docs stage site is ready! Visit it here: https://unodocsprstaging.z13.web.core.windows.net/pr-15196/index.html

@unodevops
Copy link
Contributor

🤖 Your WebAssembly Sample App stage site is ready! Visit it here: https://unowasmprstaging.z20.web.core.windows.net/pr-15196/index.html

@ramezgerges
Copy link
Contributor Author

It seems that the first test among the newly enabled tests always fails. Initially, CI was failing because of When_MediaPlayerElement_SetIsFullWindow_Check_Fullscreen. I disabled it and now When_MediaPlayerElement_SetSource_Check_Play is not passing (it was passing in CI when When_MediaPlayerElement_SetIsFullWindow_Check_Fullscreen was enabled). I suspect it may have to do with caching the sample video file after the first test, so the later tests are done before the timeout hits (although that would be weird as well since I've increased the timeouts to be 20secs instead of 8, it's just a 5mb file). I will test with a different host of the same video.

@jeromelaban jeromelaban marked this pull request as draft February 1, 2024 13:53
{
return true;
}
return true;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jeromelaban What do you think about this? I'm assuming you added this initially so that SarDen being non-zero marks the preparation of a video. This would make sense, but we don't need to wait for SarDen to start layoutting. The details (width and height) can be present even though SarDen is not filled out yet. I've failed to pinpoint exactly when or why SarDen is filled, but proceeding without SarDen temporarily seems to fix the tests (and also works when I've tested manually).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, I think the original might be right. This change makes it so that the MPE takes space for the video even though it hasn't loaded yet, filling with black until the playback starts. I will need to think about this further.

Copy link
Member

Choose a reason for hiding this comment

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

I recall that checking for SarDen was about waiting for the natural size of the video to be available.

@unodevops
Copy link
Contributor

🤖 Your Docs stage site is ready! Visit it here: https://unodocsprstaging.z13.web.core.windows.net/pr-15196/index.html

@unodevops
Copy link
Contributor

🤖 Your WebAssembly Sample App stage site is ready! Visit it here: https://unowasmprstaging.z20.web.core.windows.net/pr-15196/index.html

@ramezgerges ramezgerges marked this pull request as ready for review February 2, 2024 09:06
@ramezgerges ramezgerges marked this pull request as draft February 2, 2024 09:53
@ramezgerges
Copy link
Contributor Author

I came back to this PR today and I think the fix is only partially correct. We should not be proceeding until SarDem/SarNum are filled in (i.e. are not zero). However, for the tests to pass (and to match WinUI):

  1. The video needs to load regardless of whether or not it's autoplayed. On WinUI, the video will load and fill the space (showing the first frame) even if AutoPlay is false (and the video is not played).
  2. In LibVLC, this is very problematic to implement. There is no method to " just load the first frame". We will need some trickery to start playing the video, but then immediately pause and go back to the first frame.
  3. Since most of VLC's methods don't take effect immediately (not async, they just return immediately but the effect happens later), and since VLC isn't in sync with our UI thread, actually implementing this proved to be very difficult. Every attempt I tried resulted in a race condition one way or the other.

So, we can either proceed with the PR in its current state (fill the space of the video with black and don't load anything until we actual start playing) or we can wait for a way to implement the more correct behaviour.

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

4 participants