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

Open
wants to merge 18 commits into
base: main
Choose a base branch
from

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

@snoesberger
Copy link
Contributor

snoesberger commented Jun 7, 2024

Or why don't we just remove the checkbox and image completely? If there is no audio track we shouldn't show one. Instead we could just show a "rectangle" as a placeholder:

Might be because of my developer glasses, but to me that looks like the page is broken :D

You may be right, this could be confusing for some users. But if we show something for the missing audio track, I don't think the user should be able to interact with that section. Therefore I resume:

  • the checkbox for the track with no audio should be deselected
  • it should not be possible to select the checkbox for the track with no audio
  • instead of a placeholder image just a text like "No audio track available" could be displayed
  • video and audio boxes should have the same size and be aligned

@luniki
Copy link

luniki commented Jun 14, 2024

Or why don't we just remove the checkbox and image completely? If there is no audio track we shouldn't show one. Instead we could just show a "rectangle" as a placeholder:

Might be because of my developer glasses, but to me that looks like the page is broken :D

You may be right, this could be confusing for some users. But if we show something for the missing audio track, I don't think the user should be able to interact with that section. Therefore I resume:

* the checkbox for the track with no audio should be deselected

* it should not be possible to select the checkbox for the track with no audio

* instead of a placeholder image just a text like "No audio track available" could be displayed

* video and audio boxes should have the same size and be aligned

This patch adresses all of the todo items:
0001-Align-audio-tracks-and-add-descriptive-text-when-mis.patch.txt

Screenshot:
Screenshot 2024-06-14 at 12-54-39 Opencast Editor

We could hide the checkbox if the audio is missing:
Screenshot 2024-06-14 at 12-54-59 Opencast Editor

Copy link

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

@snoesberger
Copy link
Contributor

Thanks for the improvements! I can confirm that the following points are now fixed:

  • the checkbox for the track with no audio should be deselected
  • it should not be possible to select the checkbox for the track with no audio
  • instead of a placeholder image just a text like "No audio track available" could be displayed
  • video and audio boxes should have the same size and be aligned

The problem where a placeholder image is displayed instead of the waveform is still open:
grafik
@Arnei have you had a chance to look at it yet?

@Arnei
Copy link
Member Author

Arnei commented Jun 19, 2024

The problem where a #1230 (comment) is still open:

This is a result of a more general problem, where the editor does not put too much effort (read: pretty much no effort) into associating waveform(s) with their respective videos. This was fine originally, as other views beside the track selection only ever required one waveform and it did not matter which video it came from.

Fixing this properly would require a bit of rewriting, also in how we send waveforms from the backend to editor. Not sure if this should be fixed in this PR, or a separate one.

@Arnei
Copy link
Member Author

Arnei commented Jun 19, 2024

ToDo Notes:
The remaining ToDos are mostly in the backend. I'll still document them here in the frontend, because this is the "main" issue where discussion happens.

  • Community Workflows can't handle Audio Only. There is an open PR that intends to solve that, but that seems to be running into it's own problems: Add support for processing and playing audio only files opencast#5856
    • Alternatively, we could add a configuration option here in the frontend to disallow Audio Only track selections by default.
  • Backend cannot do audio muxing. The track-selection WOH that is normally used for handling audio for track selections does not support this.
    • Alternatively, don't do audio muxing? From what I've heard, most people use duplication anyway?
  • Backend cannot handle 3+ videos. The track-selection WOH explicitly only supports two videos. Supporting more would require a rewrite of the WOH.
    • Alternatively, for now, we could disable the track selection in the frontend if there are 3+ videos.

We generally can't server assets from the
public directory in Opencast, so this changes it
so that we import the placeholder png as URL.
Copy link

This pull request is deployed at test.editor.opencast.org/1230/2024-06-19_09-41-55/ .
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 Jun 21, 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 Jun 21, 2024
Copy link

This pull request is deployed at test.editor.opencast.org/1230/2024-06-21_10-43-45/ .
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 Jun 25, 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 Jun 26, 2024
Copy link

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

Adds a new config option to track selection. Instead
of ensuring that at least any one stream remains
enabled, this option makes is so that at least any
one *video* stream remains enabled. This is useful
if your Opencast cannot handle audio only.

Also makes this new config option the default.
Copy link

This pull request is deployed at test.editor.opencast.org/1230/2024-06-27_09-04-50/ .
It might take a few minutes for it to become available.

Adds a new config option to track selection. It is
true per default and disables the track selection
for events with more than two videos. This is
because standard Opencast workflows are not
equipped to deal with track selection for more
than two videos.
Copy link

This pull request is deployed at test.editor.opencast.org/1230/2024-06-27_09-10-39/ .
It might take a few minutes for it to become available.

This changes wording in the info box to better reflect
how the community workflows will handle the
given selection. I am aware that this is not what
we discussed, but it is the current default and what
therefore most users arguably will expect to happen.
Copy link

This pull request is deployed at test.editor.opencast.org/1230/2024-06-27_09-18-32/ .
It might take a few minutes for it to become available.

@Arnei
Copy link
Member Author

Arnei commented Jun 27, 2024

I handled the ToDos from my last comment by going for the mentioned alternatives.

This means that

  • Whether audio only should be allowed is now configurable (NOT ALLOWED by default)
  • Whether the track selection should be shown for events with +3 videos (NOT SHOWN by default)
  • Info text has been changed in the following ways:
    • User is not told anymore that multiple audio tracks will be mixed.
    • User is now told that audio track will be duplicated onto all videos, if exactly one audio track is selected.

While these "solutions" are not what exactly what we wanted, they do accurately reflect the current state of the Opencast backend and workflows. Furthermore, solving these ToDos "properly" (make OC workflows handle audio only, make select-tracks WOH handle +3 videos, implement audio muxing) is a lot of work that likely will not happen anytime soon, and I would hate to see this PR get blocked because of that. Even if this PR does not achieve all the things we set out to do, it still constitutes a considerable improvement. I would therefore like to see it merged, and create follow-up issues for the "proper" solutions.

@snoesberger
Copy link
Contributor

While these "solutions" are not what exactly what we wanted, they do accurately reflect the current state of the Opencast backend and workflows. Furthermore, solving these ToDos "properly" (make OC workflows handle audio only, make select-tracks WOH handle +3 videos, implement audio muxing) is a lot of work that likely will not happen anytime soon, and I would hate to see this PR get blocked because of that. Even if this PR does not achieve all the things we set out to do, it still constitutes a considerable improvement. I would therefore like to see it merged, and create follow-up issues for the "proper" solutions.

I completely agree with you. I also prefer a first version with the basic functionalities that we can already use. The rest of the customisation can be done in separate PRs.

@snoesberger
Copy link
Contributor

@lkiesow do you also agree with the procedure described by @Arnei in this comment?
I'd love to see this PR merged, because then we could add the translations to Crowdin and use this feature properly in production.

@snoesberger
Copy link
Contributor

We have now been running this PR in production for almost two months without any problems. From our user point of view (without being able to judge the code quality), this PR could be merged.

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
5 participants