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

Paella 7 mp4 quality selector #5685

Merged
merged 3 commits into from
May 1, 2024

Conversation

miesgre
Copy link
Contributor

@miesgre miesgre commented Mar 20, 2024

Your pull request should…

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.

Code change looks good to me!

As context: this is the result of work in the past few months. After some back and forth, some changes to paella-core were made, but the majority of this quality switching logic is part of a new external plugin mp4MultiQualityVideoFormat.

There is a Tobira PR with the exact same change. That PR has a test deployment and a couple comments detailing how this was tested already. Sascha found one small problem with Firefox in some scenarios, but that doesn't need to block this in our opinion.

@snoesberger
Copy link
Contributor

I did the same tests for this PR as I did already for the Tobira PR on my MacBook Pro with Safari, Chrome, Firefox and Opera and the quality selection works as expected.
As Lukas already mentioned, the really small problem with Firefox doesn't need to block this PR.

Thanks for the implementation!

Copy link
Member

@Arnei Arnei left a comment

Choose a reason for hiding this comment

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

Tested it briefly with a develop install of Opencast and it works. Code changes to Opencast are straightforward enough :)

It is arguably unintuitive that the quality selector does not pop up if there only is one quality, or at least it had me wondering if I did something wrong. But that's no reason to block this.

@Arnei
Copy link
Member

Arnei commented Apr 10, 2024

By the way, we probably want this to go into earlier branches than develop. I'd say r/15.x at least, but maybe r/14.x could also work?

@miesgre
Copy link
Contributor Author

miesgre commented Apr 10, 2024

By the way, we probably want this to go into earlier branches than develop. I'd say r/15.x at least, but maybe r/14.x could also work?

paella-core needs to be updated to 1.48.1

  • r/14.x uses paella-core@1.43.0
  • r/15.x uses paella-core@1.46.1

If there is no problem in updating the version of paella-core in r/14.x and r/15.x, I can rebase to r/14.x or r/15.x.

@Arnei
Copy link
Member

Arnei commented Apr 11, 2024

I don't know how Paella does it its versioning, but since there are no major version changes it should be fine? Probably better to just test. I guess you would now better if updating paella-core is gonna cause inter-dependency issues in r/15.x or r/14.x ^^

@KatrinIhler
Copy link
Member

I'd probably aim this at 15, but I don't know how we handled paella updates in the past tbh.

@miesgre
Copy link
Contributor Author

miesgre commented Apr 12, 2024

I don't know how Paella does it its versioning, but since there are no major version changes it should be fine? Probably better to just test. I guess you would now better if updating paella-core is gonna cause inter-dependency issues in r/15.x or r/14.x ^^

I should be secure to update from paella-core@1.46.1 to paella-core@1.48.1.
I made a rebase and to r/14.x and create a new branch to test. In my test it works as expected. Please test it.
You can find the branch here: https://github.com/miesgre/opencast/tree/paella-mp4-quality-selector-r14x

The config file needs to be updated to use the quality change.
The PR updated the config file, but if some institution is not using the default, will need to update it.

Disable the es.upv.paella.mp4VideoFormat and add the es.upv.paella.mp4MultiQualityVideoFormat.

{
    "plugins": {
        ...
        "es.upv.paella.mp4VideoFormat": {
            "enabled": false,
            "order": 1
        },
        "es.upv.paella.mp4MultiQualityVideoFormat": {
            "enabled": true,
            "order": 1
        },
      }
        ... other plugin settings
    }
}

Copy link
Contributor

This pull request has conflicts ☹
Please resolve those so we can review the pull request.
Thanks.

@KatrinIhler
Copy link
Member

Result of the Technical Meeting: Could probably go into 14, but since it's soon EOL and it would be a "big" jump, let's aim at 15 instead.

@miesgre miesgre force-pushed the paella-mp4-quality-selector branch from 2f912d6 to c2d3219 Compare April 16, 2024 16:48
@miesgre miesgre changed the base branch from develop to r/15.x April 16, 2024 16:49
@miesgre
Copy link
Contributor Author

miesgre commented Apr 16, 2024

Result of the Technical Meeting: Could probably go into 14, but since it's soon EOL and it would be a "big" jump, let's aim at 15 instead.

Rebased to r/15.x

@gregorydlogan gregorydlogan self-assigned this May 1, 2024
@gregorydlogan gregorydlogan merged commit 5349c4a into opencast:r/15.x May 1, 2024
1 check passed
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.

Paella 7: Switch between video resolutions no longer possible for progressive download video streaming
6 participants