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

Split track selection to video and audio selection #1230

Merged
merged 27 commits into from
Jan 16, 2025

Conversation

Arnei
Copy link
Member

@Arnei Arnei commented Dec 14, 2023

In the "Tracks" view, instead of de-/selecting the whole track, the video and audio streams can be de-/selected individually. At least one video stream must remain selected. All audio streams can be deselected. If the audio stream is marked as unavailable, it cannot be de-/selected.

Shows the waveform from the timeline for a graphical representation of the audio stream. However, there will only be one waveform generated for the timeline, so in case of additional tracks a placeholder image will be shown instead. Any suggestions/submissions for a better placeholder graphic are very welcome.

Resolves #1009. #1009 also mentions potential issues in the backend, while this PR only addresses the frontend. From my limited testing the backend seems fine when using the default community workflows (no errors, the correct streams get published). If there are any users around that are still using the track selection feature in the old editor, and that are aware of any backend issues, please do tell me about them.

Courtesy of the Opencast 2023 Crowdfunding.

In the "Tracks" view, instead of de-/selecting the whole track,
the video and audio streams can be de-/selected individually.
At least one video stream must remain selected. All audio streams
can be deselected. If the audio stream is marked as unavailable,
it cannot be de-/selected.

Shows the waveform from the timeline for a graphical representation
of the audio stream. However, there will only be one waveform
generated for the timeline, so in case of additional tracks a
placeholder image will be shown instead.

Resolves opencast#1009. opencast#1009 also mentions potential issues in the backend,
while this PR only addresses the frontend. From my limited testing
the backend seems fine when using the default community workflows
(no errors, the correct streams get published). If there are any users
around that are still using the track selection feature in the old
editor, and that are aware of any backend issues, please do tell me
about them.
@Arnei Arnei added the type:enhancement New feature or request label Dec 14, 2023
Copy link

This pull request is deployed at test.editor.opencast.org/1230/2023-12-14_10-06-39/ .
It might take a few minutes for it to become available.

@lkiesow
Copy link
Member

lkiesow commented Jan 4, 2024

There are definitely issues in the backend. But I also see usability issues in the frontend.

For example, what will be the result of a selection like this?
Screenshot from 2024-01-04 01-16-17

As a user, my assumption would be to get either a single video with mixed audio streams from the other tracks or three tracks, two with audio and one with video.

However, the muxing command being executed is:

[ffmpeg, -nostdin, -nostats, -i, ...fc9.mp4, -i, ...ad4.mp4, -shortest, -c, copy, -map, 0:v:0, -map, 1:a:0, ...b5-copy.mp4]

This completely discards one of the selected audio streams. As a user, I can just describe that as a bug.

And then, what would be the expected result of something like this?

Screenshot from 2024-01-04 01-31-51

Will all audio streams be mixed into a single audio stream and applied to A? Or will just C be transferred to A? As a user, you have no idea.

@Arnei
Copy link
Member Author

Arnei commented Jan 4, 2024

My thoughts on how to go about the usability issues you highlighted:

  1. Leave it as is, people will "somehow just know" how it works at their place. Arguably workable, but not a great solution.

  2. Introduce restrictions on what combination of tracks can and cannot be selected. Would need to be configurable, because there will always be admins that want to allow combinations that others would restrict, like the ones you mentioned. But then we again run into the issue of having to "just know" what certain combinations will do.

  3. Add more buttons/checkboxes/ui that describes what should happen with the tracks, e.g. Mux all remaining audio streams onto A/B/C or Keep audio on tracks, Allow tracks with only audio. Arguable the way to go even if it muddles up the UI somewhat, but also requires an editor workflow to support all those new flags.

@lkiesow
Copy link
Member

lkiesow commented Jan 4, 2024

The original goal of the editor was to be simple since it is meant for non-technical end-users. I would really like to keep that goal since we expose the editor to such users.

Therefor, a lot of implicit knowledge as a requirement isn't a great solution and neither is having a lot of boxes and buttons with technical terms.

As an alternative, we could introduce one simple way to handle the result. My suggestion would be: If you remove any audio stream, all other audio streams will be mixed and attached to all videos. This rule is simple and works with as many videos tracks as you want. It also works well with other parts of Opencast like the Paella Player, which will just play back a single stream.

Running with that idea, we would need a change in the backend workflow operation and a hint about how this works in the UI of this pull request. The hint should also include that the mixing will happen if a track is already missing an audio stream.

Furthermore, since users should be able to deal with individual audio streams, we also need to make sure the editor can playback all streams, and individual streams. A simple solution may be playback controls on the tracks page where you then disable the audio streams of the tracks were audio has been removed?

@github-actions github-actions bot added the status:conflicts Conflicts with another pull request or issue label Jan 18, 2024
Copy link

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

@github-actions github-actions bot removed the status:conflicts Conflicts with another pull request or issue label Jan 18, 2024
Copy link

This pull request is deployed at test.editor.opencast.org/1230/2024-01-18_15-31-01/ .
It might take a few minutes for it to become available.

@KatrinIhler
Copy link
Member

Finally getting around to commenting on this... I will not chime in with the ongoing discussion, but just say that

At least one video stream must remain selected

is something I would like to see either being configurable in the backend, or simply not enforced (which I think you already knew I was gonna say ;)), because it doesn't take into account audio-only use cases. Consider the following:

  1. An institution with custom workflows for publication that can handle audio-only input (either by rendering a static image into it, or by having a player that can handle audio-only), and
  2. A recording where the video is either black or otherwise damaged or unusable, but the audio is fine, and I want to deselect the video and publish the audio only with my custom workflow, or
  3. A recording that doesn't come with a video stream in the first place that I want to cut and publish.

Both of these cases should be possible with the new editor. It doesn't have to be the case out of the box, but I shouldn't need to patch the code to make this work, just do some configuration. So basically the same arguments I made here, except that for the new editor, making this configurable with an "allow audio only in editor" or something would actually be worth it.

@lkiesow lkiesow self-assigned this Feb 16, 2024
@github-actions github-actions bot added the status:conflicts Conflicts with another pull request or issue label Mar 1, 2024
Copy link

github-actions bot commented Mar 1, 2024

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

Copy link
Member

@KatrinIhler KatrinIhler left a comment

Choose a reason for hiding this comment

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

See my comment above, also conflicts. :(

@luniki
Copy link

luniki commented May 22, 2024

Lars sent me this "oneliner" to create a triple event:

curl -u admin:opencast "${SERVER}/ingest/addMediaPackage/fast" \
        -F 'flavor=presenter/source' \
        -F mediaUri=https://example.com/opencast/test-media/goat.mp4 \
        -F 'flavor=presentation/source' \
        -F mediaUri=https://example.com/opencast/test-media/goat.mp4 \
        -F 'flavor=experiment/source' \
        -F mediaUri=https://example.com/opencast/test-media/goat.mp4 \
        -F identifier=three \
        -F title="Three Video Streams" \
        -F creator="$(shuf -n 1 /usr/share/dict/words)"

@Arnei
Copy link
Member Author

Arnei commented May 22, 2024

Results of todays discussion between Sascha, Katrin, Lars, Arne and Marcus:

  • Should be able to select 0, 1 or more videos
  • In the same vein, should be able to select 0, 1 or more audios
  • At least one track (video or audio) needs to remain
  • The same audio is applied to all videos: None, one or multiple muxed
    • Have a note in the UI that informs the user about muxing
  • In case no video remains selected, the remaining audios are muxed into one audio track
  • Default selection when opening up the tracks view: All videos are selected and only the first audio track is selected
    • ...why did we want this again?
  • There should be a big confirm button
    • This idea here is to avoid a user making an accidental change to their event

Key-Image
Bildschirmfoto vom 2024-05-22 15-46-30

@luniki
Copy link

luniki commented May 29, 2024

I talked to @Arnei about this and created a first version of it. Here are some screenshots.

Initial state after opening the "Tracks" tab

Screenshot 2024-05-29 at 12-04-11 Opencast Editor

After de-selecting a video track

Screenshot 2024-05-29 at 12-08-49 Opencast Editor

After de-selecting all video tracks

Screenshot 2024-05-29 at 12-11-03 Opencast Editor

After de-selecting all but one track

Screenshot 2024-05-29 at 12-13-48 Opencast Editor

@luniki
Copy link

luniki commented May 29, 2024

The "Confirm selection" buttons seems to be useless. Last we talked we defined:

There should be a big confirm button
* This idea here is to avoid a user making an accidental change to their event

The initial state is that all tracks are selected. This can also be easily determined in the code. So if we never de-select any tracks, we don't change the event?!

@snoesberger
Copy link
Contributor

The "Confirm selection" buttons seems to be useless. Last we talked we defined:

There should be a big confirm button

  • This idea here is to avoid a user making an accidental change to their event

The initial state is that all tracks are selected. This can also be easily determined in the code. So if we never de-select any tracks, we don't change the event?!

IIRC, the idea was not to have a confirm button, but to have a checkbox to activate the track selection. So if a user wants to de-select tracks, they first have to activate this "modify track selection" checkbox. As long as the checkbox is inactive, video and audio selection is grayed-out, nothing can be changed and the track selection will be completely ignored. As soon as the checkbox is activated, the user will be able to de-select video and/or audio tracks, and Opencast will process video and audio accordingly (muxing audio, etc.) as soon as the user starts the workflow. @KatrinIhler and @lkiesow please correct me if I'm wrong.

@KatrinIhler
Copy link
Member

Agreed with @snoesberger completely . The difference between an activated and deactivated track selection, if the user does not do anything else, is whether the audio tracks are kept as-is or are muxed.

So no to the confirm button (bc this doesn't persist this decision in a visible way later), yes to a checkbox or something to activate the whole selection thingy (as shown on the initial sketch).

luniki pushed a commit to luniki/opencast-editor that referenced this pull request May 31, 2024
@luniki
Copy link

luniki commented May 31, 2024

I have pushed my changes to this PR to my own branch: https://github.com/luniki/opencast-editor/commits/tracks-select-audio/

@luniki
Copy link

luniki commented May 31, 2024

Here are some screenshot of my current state of track selection.

Entering the track selection

image

Enabled track selection

image

De-selected all but one track

image

@luniki
Copy link

luniki commented May 31, 2024

The checkbox labelled "Customize track selection" is a new part of the state of the editor which has to be transferred to the Opencast EditorService endpoint. At the moment there is an additional boolean key in the JSON object.

This requires changes in the EditorRestEndpointBase class (and the GSON classes).

Or do I get this wrong? How should that checkbox be encoded?

@luniki
Copy link

luniki commented Jun 4, 2024

I pushed my changes to my forked branch of opencast-editor: https://github.com/luniki/opencast-editor/commits/tracks-select-audio/

In the backend I edited the EditingData class: https://gitlab.elan-ev.de/mlunzena/opencast-bern/-/commits/feature/track-selection

Copy link

github-actions bot commented Dec 6, 2024

This pull request is deployed at test.editor.opencast.org/1230/2024-12-06_08-06-22/ .
It might take a few minutes for it to become available.

@github-actions github-actions bot added the status:conflicts Conflicts with another pull request or issue label Dec 9, 2024
Copy link

github-actions bot commented Dec 9, 2024

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

@github-actions github-actions bot removed the status:conflicts Conflicts with another pull request or issue label Dec 10, 2024
Copy link
Member

@JulianKniephoff JulianKniephoff left a comment

Choose a reason for hiding this comment

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

src/main/TrackSelection.tsx Outdated Show resolved Hide resolved
src/main/TrackSelection.tsx Outdated Show resolved Hide resolved
src/main/TrackSelection.tsx Outdated Show resolved Hide resolved
As suggest by @JulianKniephoff. Fixes import
of appkit css styles.
Copy link

This pull request is deployed at test.editor.opencast.org/1230/2024-12-12_16-32-25/ .
It might take a few minutes for it to become available.

Copy link

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

@github-actions github-actions bot added the status:conflicts Conflicts with another pull request or issue label Dec 17, 2024
@github-actions github-actions bot removed the status:conflicts Conflicts with another pull request or issue label Dec 17, 2024
Copy link

This pull request is deployed at test.editor.opencast.org/1230/2024-12-17_08-03-28/ .
It might take a few minutes for it to become available.

@github-actions github-actions bot added the status:conflicts Conflicts with another pull request or issue label Dec 17, 2024
Copy link

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

@github-actions github-actions bot removed the status:conflicts Conflicts with another pull request or issue label Dec 17, 2024
Copy link

This pull request is deployed at test.editor.opencast.org/1230/2024-12-17_10-05-16/ .
It might take a few minutes for it to become available.

Copy link

github-actions bot commented Jan 8, 2025

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

@github-actions github-actions bot added the status:conflicts Conflicts with another pull request or issue label Jan 8, 2025
@github-actions github-actions bot removed the status:conflicts Conflicts with another pull request or issue label Jan 9, 2025
Copy link

github-actions bot commented Jan 9, 2025

This pull request is deployed at test.editor.opencast.org/1230/2025-01-09_08-23-25/ .
It might take a few minutes for it to become available.

@marwyg
Copy link
Member

marwyg commented Jan 15, 2025

I overlooked some GitHub warnings in my last commit. Now everything should be fixed. @Arnei I created a PR against your Branch again: Arnei#271. Besides my successful tests, this feature is already running on a production system (since last summer I think), so it seems to work fine.

I also took a look at the code. Besides the GitHub warnings (that should be hopefully dealt with by now) , the code looks good to me.

I think the PR is ready to be merged.

Copy link

This pull request is deployed at test.editor.opencast.org/1230/2025-01-15_14-43-59/ .
It might take a few minutes for it to become available.

@KatrinIhler KatrinIhler merged commit f4c7f68 into opencast:main Jan 16, 2025
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove video or audio from a track
7 participants