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 quality selector for progressive download videos #1143

Merged
merged 3 commits into from
Apr 2, 2024

Conversation

LukasKalbertodt
Copy link
Member

@LukasKalbertodt LukasKalbertodt commented Mar 13, 2024

After much discussion, some relevant changes have been merged into paella-core and the other part is published as this package. So we can use this modified video format plugin to get back mp4 quality selection.

I have tested it a bunch and so far everything works fine. But please everyone: test this a bunch on different devices and different browsers and report any problems or weird behaviors here. Ideally test a number of different videos (single & dual stream). Note: The "The best open cat videos" do not have different qualities, so you can ignore those.

Test here

There are two known shortcomings still:

  • When changing quality, both videos are loaded sequentially, which is slower in most cases than loading them at the same time. To fix this, changes in paella-core are necessary. These might be added in the future.
  • There is no spinner or any UI at all indicating to the user that the new quality video is being loaded. The playback stops and potentially the videos are going black and then showing a frame again, but this is not ideal from a UX perspective. However, having the quality changing feature at all was the priority, and this might be added in the future still.

This pull request also changes the logic for selecting the initial quality. Before, the maximum quality was always chosen, which is really not ideal most of the time. You can find some additional information in this commit description. Please also test this change and let me know if you encounter a situation where the initial quality was a bad choice. Also note: the new logic still errs on the side of "high quality". I'm sure for many institutions it might be useful to lower the initial quality even more.

@LukasKalbertodt LukasKalbertodt added the changelog:user User facing changes label Mar 13, 2024
@github-actions github-actions bot temporarily deployed to test-deployment-pr1143 March 13, 2024 09:29 Destroyed
@LukasKalbertodt
Copy link
Member Author

LukasKalbertodt commented Mar 13, 2024

Things I tested:

  • Changing quality while paused: should remain paused until manually resumed.
  • Changing quality while playing: should pause until the new quality is loaded and then automatically resume.
  • During quality change, it shouldn't show/flash the first frame of the video. Showing black briefly is not ideal but fine.
  • All these things shouldn't change after changing the quality: playback rate (speed), volume, ...

But try to come up with test conditions for yourself, I likely forgot something here!

I tested:

  • Linux; 4k screen; Firefox and Chrome: works (initial quality: 2160p)
  • Windows 10; 1440p screen; Firefox, Chrome and Edge: works (initial quality: 1080p, 1440p wasn't availabe in the videos I tested). Firefox sometimes flashed a wrong frame during quality change, but I couldn't properly reproduce that :/ Shouldn't block merging this I think.
  • iPhone 13 Mini (Safari): works (initial quality: 720p). For one video, a slightly zoomed in version of the video is shown for a split second after the quality change. I'm not exactly sure what I'm seeing. Also shouldn't block merging this I think.

I also noticed that the new fake fullscreen mode (used on the iPhone) makes it impossible to see Paella menus (like the quality or speed). This has nothing to do with this PR, but I will try to fix it. Fixed

@github-actions github-actions bot temporarily deployed to test-deployment-pr1143 March 13, 2024 11:25 Destroyed
@owi92
Copy link
Member

owi92 commented Mar 13, 2024

I did some testing as well with Firefox, Chrome, Safari and my ageing android phone.
I noticed one thing that I can only reproduce on this test deployment:

Other than that, my testing suggests that this seems to work as intended.

@LukasKalbertodt
Copy link
Member Author

Some videos may start while the loading icon is still shown. This happens for example with https://pr1143.tobira.opencast.org/!v/JcU4iuqj4LT and https://pr1143.tobira.opencast.org/v/HO499MHSQuR
As far as I can tell this only happens with dual stream videos, though I didn't test huge variety. Can you check whether you can reproduce this?

I can. Dang, I saw this before but assumed this was already the case before. Will investigate next week.

@LukasKalbertodt
Copy link
Member Author

Okay, this is not caused by my changes to the quality logic, but by another unrelated change, that landed between 1.46.6 and 1.47.1. And since this PR updates paella-core, the bug is included. I created an upstream PR: polimediaupv/paella-core#359

Once that is merged and released, I will update this PR.

@snoesberger
Copy link

I did test this on my MacBook Pro with Safari, Chrome, Firefox and Opera. The testcases @LukasKalbertodt mentioned did all work well. But I do have one small issue with the video https://pr1143.tobira.opencast.org/v/AYrn0crKqY1. If I switch the quality in Firefox, for a short moment the first video frame (or is it the preview image?) is shown. This happens only for this video and only in Firefox on my MacBook. If I open this video on my iPhone 12 Mini in Firefox, it is working as expected.

On my iPhone 12 Mini the tests were also successful both with Safari and Firefox.

This has been a journey, but now this lives in an external plugin that
can be used in place of the standard mp4 video format plugin.
These manual z-index changes are terrible of course. But I don't see a
better solution.
@LukasKalbertodt
Copy link
Member Author

@snoesberger Thanks for the tests. I can reproduce the Firefox issue. I started working on a fix and I think I have an interesting approach (for devs: creating a new video element via oldVideoElement.cloneNode(), setting the new src, waiting for "canplay" event, then oldVideoElement.replaceWith(newVideoElement)). This might be better in many ways anyway, but it requires more work and is also finicky to get right (cloneNode() does not clone event handlers, nor volume, and potentially other things). So in the interest of time and priorities of tasks, I would suggest not addressing this issue for now. Is that fine by you?

@snoesberger
Copy link

So in the interest of time and priorities of tasks, I would suggest not addressing this issue for now. Is that fine by you?

That is fine for me, we can leave it like that for the moment.

@LukasKalbertodt
Copy link
Member Author

With 1.48.1, the problem that @owi92 found seems to be resolved. If you can confirm that Ole, I think this is ready to be merged.

@github-actions github-actions bot temporarily deployed to test-deployment-pr1143 March 26, 2024 13:20 Destroyed
@owi92
Copy link
Member

owi92 commented Apr 2, 2024

With 1.48.1, the problem that @owi92 found seems to be resolved.

Can confirm.

@owi92 owi92 merged commit ae926af into elan-ev:master Apr 2, 2024
3 checks passed
@LukasKalbertodt LukasKalbertodt deleted the paella-quality-selector branch April 2, 2024 09:11
@oas777
Copy link
Collaborator

oas777 commented May 2, 2024

Late to the game and moving on thin ice: In the old ETH video portal, we don't offer quality selection in "embed mode", because the space available for the video has less pixels available than the higher resolution one might chose. And we don't expect anyone to chose lower resolutions in a scenario like https://video.test.tobira.ethz.ch/!v/AUGZS5hMGIy. So maybe it makes sense to restrict the quality selector for fullscreen scenarios?

@oas777
Copy link
Collaborator

oas777 commented May 2, 2024

PS:
grafik
Why?

@LukasKalbertodt
Copy link
Member Author

Late to the game and moving on thin ice: In the old ETH video portal, we don't offer quality selection in "embed mode", because the space available for the video has less pixels available than the higher resolution one might chose. And we don't expect anyone to chose lower resolutions in a scenario like https://video.test.tobira.ethz.ch/!v/AUGZS5hMGIy. So maybe it makes sense to restrict the quality selector for fullscreen scenarios?

I assume by "embed mode" here you just mean "not fullscreen mode"? I don't quite yet understand the reasoning for hiding the quality selector there, but in any case: in Tobira, the player is much larger in "non-fullscreen mode", so is much closer to fullscreen and thus I think that a quality selector certainly makes sense there too.

Why?

Are you referring to the unusual resolutions?

@oas777
Copy link
Collaborator

oas777 commented May 8, 2024

I assume by "embed mode" here you just mean "not fullscreen mode"? I don't quite yet understand the reasoning for hiding the quality selector there, but in any case: in Tobira, the player is much larger in "non-fullscreen mode", so is much closer to fullscreen and thus I think that a quality selector certainly makes sense there too.

The reasoning was/is that we provide more pixels (here: 1920x1080) than the regular monitor will allow you to use in that space. But maybe that's me staring at my laptop monitor all the time.

And yes, 576 seems an odd resolution.

@LukasKalbertodt
Copy link
Member Author

The reasoning was/is that we provide more pixels (here: 1920x1080) than the regular monitor will allow you to use in that space. But maybe that's me staring at my laptop monitor all the time.

Ah I see. But as I said: I think it's quite different in Tobira, as the non-fullscreen player is already larger. I think conditionally hiding the quality selection will only cause more confusion.

And yes, 576 seems an odd resolution.

This is actually one of the standard vertical-resolution of DVDs! https://en.wikipedia.org/wiki/DVD-Video
But yeah anyway, not a resolution one certainly ones on your video portal. But luckily (for me :D) the provided resolutions come from Opencast, and Tobira cannot do anything about it. So yeah, if your Opencast provides nicer resolutions, then this is not an issue for you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog:user User facing changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants