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 paella dual stream layout #985

Merged
merged 1 commit into from
Nov 8, 2023

Conversation

owi92
Copy link
Member

@owi92 owi92 commented Nov 6, 2023

Closes #956

Before:
Bildschirmfoto 2023-11-06 um 14 06 18

After:
Bildschirmfoto 2023-11-06 um 14 05 56

Two unexpected sideeffects:

  • The "fullscreen" and "swap" icons somehow changed. Huh? this is desired and simply part of the dynamic layout it appears. Well it can be configured as I understand, but these seem to be the default.
  • Default layout is now "Presenter" for some reason. Which is unfortunate. With this, users might think dual stream isn't working. There must be a way to configure that but I couldn't find it yet.

@github-actions github-actions bot temporarily deployed to test-deployment-pr985 November 6, 2023 13:19 Destroyed
@owi92 owi92 added the changelog:user User facing changes label Nov 6, 2023
@oas777
Copy link
Collaborator

oas777 commented Nov 6, 2023

"The "fullscreen" and "swap" icons somehow changed. Huh?"

This is part of the Paella confusion: These are the icons of P7 delivered with the latest Opencast, cf. https://stable.opencast.org/paella7/ui/watch.html?id=ID-dual-stream-demo

Not to be confused with P7 from https://paellaplayer.upv.es/.

Or https://polimediaupv.github.io/paella-ethz/?id=belmar_16_9_hls, which has the latest and greatest design. Or is it https://polimediaupv.github.io/paella-skins/?

@owi92
Copy link
Member Author

owi92 commented Nov 6, 2023

@oas777 Yeah I was mostly wondering what's happening "under the hood" there. But if the new icons are the ones you prefer, at least that side effect is a good one, I suppose.

@oas777
Copy link
Collaborator

oas777 commented Nov 6, 2023

If by "new icons" you mean the ones at https://tobira.opencast.org/events/opencast/2018/vienna/v/P7ha7_AnBU4: No, that's not a good side effect. The ones at https://polimediaupv.github.io/paella-ethz/?id=belmar_16_9_hls or https://polimediaupv.github.io/paella-skins/ are the ones we want.

@owi92
Copy link
Member Author

owi92 commented Nov 6, 2023

If by "new icons" you mean the ones at https://tobira.opencast.org/events/opencast/2018/vienna/v/P7ha7_AnBU4

Well no, those are the old ones, also from my perspective. I'm sorry if I didn't convey that correctly. I know that these (Bildschirmfoto 2023-11-06 um 18 07 30) are the ones you want, I was just making an offhand comment.
I was surprised that they switched with this PR's code change, which was supposed to be solely about improving space usage of dual stream videos, and isn't merged yet (see https://pr985.tobira.opencast.org/live/events/live-2 for an example).

@LukasKalbertodt
Copy link
Member

Default layout is now "Presenter" for some reason. Which is unfortunate. With this, users might think dual stream isn't working. There must be a way to configure that but I couldn't find it yet.

That should certainly be fixed before we merge this. I looked into it a bit and found out a few things that might help you:

  • My first assumption was (and maybe yours too) that in your PR, the player starts out in the "single video view" of the "dual stream dynamic layout". But that's incorrect. It actually starts out in the "single stream dynamic layout". If only one video is shown, es.upv.paella.singleVideoDynamic is active.
  • So the question is how to make Paella use the es.upv.paella.dualVideoDynamic by default (if applicable). Maybe just moving it up in the configuration already helps?
  • I also found this talking about dualVideoContentIds or something. Not sure if it helps.

@owi92
Copy link
Member Author

owi92 commented Nov 7, 2023

So there is defaultLayout: "presenter-presentation" which does what you'd expect it to do...
Man how did I miss this.

@github-actions github-actions bot temporarily deployed to test-deployment-pr985 November 7, 2023 17:36 Destroyed
Copy link
Member

@LukasKalbertodt LukasKalbertodt left a comment

Choose a reason for hiding this comment

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

Man how did I miss this.

Hey, so did I :D And I spend quite some time looking through the docs.

So yeah, seems to work, nice! And the space usage is absolutely improved, another UX win. Regarding the icons: I also don't know why this change causes the icons to change, but since we want these new icons, it's a win. So let's just merge.

@LukasKalbertodt LukasKalbertodt merged commit 79d7c44 into elan-ev:master Nov 8, 2023
2 checks passed
@owi92 owi92 deleted the dual-stream-layout branch March 4, 2024 16:36
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.

Improve space usage for dual stream videos in Paella
3 participants