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 VTT voice spans to cues #1652

Merged
merged 8 commits into from
Aug 29, 2024
Merged

Add VTT voice spans to cues #1652

merged 8 commits into from
Aug 29, 2024

Conversation

MiSikora
Copy link
Contributor

This adds voice spans available in WebVTT cues. Spans were already parsed but they were never exposed and attached to the output Spannable created by WebvttCueParser. For more info see: #1632

@oceanjules oceanjules requested review from icbaker and removed request for icbaker August 25, 2024 08:04
Copy link
Collaborator

@icbaker icbaker left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution, and thorough test changes too!

public final String speakerName;

/** The classes associated with the text. It can specify things like "first", "loud", etc. */
public final Set<String> classes;
Copy link
Collaborator

Choose a reason for hiding this comment

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

aiui these aren't really useful without access to the rest of the styling info. Do you have a use-case/need for these values?

Copy link
Contributor Author

@MiSikora MiSikora Aug 27, 2024

Choose a reason for hiding this comment

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

No I don't have a use for those. Do you want me to remove it?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, let's remove it for now, if a use-case comes up later we can re-add it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@UnstableApi
public final class VoiceSpan {

/** The speaker name. */
Copy link
Collaborator

Choose a reason for hiding this comment

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

the webvtt spec uses the phrase "voice name", so I think we can use that here:

/** The voice name. */
public final String name;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@icbaker
Copy link
Collaborator

icbaker commented Aug 29, 2024

I'm going to send this for internal review now. You may see some more commits being added as I make changes in response to review feedback. Please refrain from pushing any more substantive changes as it will complicate the internal review - thanks!

@copybara-service copybara-service bot merged commit 39ed9cf into androidx:main Aug 29, 2024
1 check passed
@androidx androidx locked and limited conversation to collaborators Oct 29, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants