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

Added speakers to internal_format #85

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

vnali
Copy link
Contributor

@vnali vnali commented Aug 25, 2023

This PR is related to #61

This PR:

  • Detects speakers from SRT and VTT files and keep it in internal_format.
  • When the VTT is created, it checks for $internal_format['speakers'] and if there is any speaker, attaches <v {speaker_name}> to the beginning and </v> to the end of the line.
  • When the SRT is created, it checks for $internal_format['speakers'] and if there is any speaker, attaches {speaker_name}: to the beginning of the line.
  • Now when add() a subtitle, you can pass the speaker as keys of third param.
    ->add(0, 1, ['{speaker_name}' => 'Line 1', 'Line 2'])
    ->add(1, 2, ['Line 1', '{speaker_name}' =>'Line2'])
    ->add(2, 3, ['{speaker_name}' => 'Line 1', '{speaker_name}' =>'Line2'])
    ->add(3, 4, ['Line 1', 'speaker2: Line2']) // If the speaker is not available -Line1- or it is already included in the line -Line2-, you should not pass speaker separately.

@mantas-done
Copy link
Owner

Nice job, I will check this commit next week :)

@mantas-done
Copy link
Owner

Refactored code, check the newest commit from the "master" branch.

Added a new class for Webvtt.
Now you can add the speaker like this (as you suggested):
(new Webvtt)->add(0, 1, ['John' => 'hello', 'hello to you too'])->...

Also changed the name of styles (vtt_cue_settings -> settings):
(new Webvtt)->add(0, 1, 'hello', ['settings' => 'position:50% line:15% align:middle'])->...

Webtt class saves everything to the vtt key:
$internal_format[0]['vtt']

For the srt file detection of the speaker is not accurate. Checked a few files and there is text like:
These colors: blue, green and red
Which would set "These colors" as a speaker.

So for now left srt as is.

What do you think?

@vnali
Copy link
Contributor Author

vnali commented Sep 2, 2023

You are definitly right about false detection of SRT files, so we must skip SRT speaker detection for now, but:

  • I am not sure if asking end users to use a separate class -Webvtt()- is a good idea because:
    • The end users should have a if logic to use Webvtt not Subtitle class if they work with different formats-
    • we could still use specifying speaker on add() for generating other types with known speaker format like SRT and Asking people to do (new Webvtt)->add(0, 1, ['John' => 'hello', 'hello to you too'])->content('srt') is weird. i have a feeling we should have a general add().
  • although as you said, we can't have an exact speaker detection for SRT file and SRT to VTT convert, but we can still correctly detect speaker from VTT files and convert VTT to SRT formats correctly, so we could still check for speaker in internal format for generating srt content like speaker: line.
  • i guess you are not happy with vtt_cue_settings on add() too, maybe we could change it to inline_cue_settings or ['vtt']['cue_settings'], ...

Please let me know if i misunderstood something
Thanks!

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.

None yet

2 participants