Feat/android subtitle event

[coofzilla]

📝 Summary

This PR is the android implementation for accessing subtitle content via the onTextTrackDataChanged method . The ios counterpart can be found here . The functionality leverages the onCues method from the Player.Listener. It adds the capability for users to control the display of subtitles through the subtitleStyles prop. It has been emphasized in the documentation that, the showSubtitles flag is to be used in conjunction with the selectedTextTrack prop; thus, allowing for selecting textTracks but deciding if you want to display them or not.

Motivation

Building on our previous efforts to improve user engagement with HLS VOD sports content through the onTextTrackDataChanged event handler, we've identified a new opportunity to enhance flexibility and control over subtitle display on Android. The introduction of the showSubtitles boolean prop is a direct response to the limitations observed in handling subtitle data. Previously, selecting tracks via the selectedTextTrack method would automatically display subtitles, offering no option to hide them while still accessing their data for other purposes, such as displaying game scores or other contextual information dynamically. This limitation was particularly noticeable when attempting to offer a clean, unobstructed view of the video content while simultaneously making use of subtitle data for enhancing viewer experience. By enabling the ability to hide subtitles even when they are selected, we empower developers to access and use subtitle data via onCues on Android (and legibleOutput on iOS) without forcing the display of subtitles on the screen.

Changes

• Subtitle Display Control: Introduced a boolean showSubtitles to control the visibility of subtitles.
• Event Handling Enhancements: Implemented the onTextTrackDataChanged event for Android, aligning with the existing iOS implementation for consistency.

📄 Documentation

- `events.md` to include the `onTextTrackDataChanged` method for Android.
- `props.md` to document the usage of the new `subtitleStyle` property.

Test plan

✅ Tested in example app with sintel with subtitles URI

Apply the following changes in the basic app:

// VideoPlayer.tsx

          // selectedTextTrack={this.state.selectedTextTrack}
          selectedTextTrack={{
            type: SelectedTrackType.TITLE,
            value: 'caption_en',
          }}
          subtitleStyle={{
            showSubtitles: false,
          }}

Comment out this line; due to textTracks being undefined, I believe it has to do with how data.textTracks is being set; but, I didn't want to address that on this PR as its a separate issue.

// VideoPlayer.tsx
  {/* {this.state.textTracks.map(track => (
    <Picker.Item
      label={track.language}
      value={track.language}
      key={track.language}
    />
  ))} */}

Comment out this line for the app to build(is there a permanent fix for this 🤔):

// metro.config.js

// /(.*\/react-native-video\/node_modules\/.*)$/,

Demo:

demo_subtitles.mp4

If you explicitly want to set them ( the default is visible):

Proof that they default to visible without having to set them:

What happens if you set showSubtitles but dont select a track?

They won't show :), this is explained in the documentation as well. This control is added for display, not setting them or not.

[freeboub]

Thank you again for the PR, the onCue part looks OK for me, but the showSubtitles shall not be added in subtitlesStyles.
In fact I expect only "normal" styling in subtitlesStyles.

To disable display of subtitle, you should selectedTextTrack={'disabled'}

Can you either remove showSubtitles or better rename it to opacity ;)

[freeboub]

You need also to fix conflict please !

[coofzilla]

Thank you again for the PR, the onCue part looks OK for me, but the showSubtitles shall not be added in subtitlesStyles. In fact I expect only "normal" styling in subtitlesStyles.

To disable display of subtitle, you should selectedTextTrack={'disabled'}

Can you either remove showSubtitles or better rename it to opacity ;)

Thank you for the time you've taken to review the PR and for your constructive feedback! After considering your comments regarding the showSubtitles flag within the subtitlesStyles, I understand the expectation for "normal" styling parameters to reside in this prop. The intention behind adding showSubtitles was to leverage the existing setVisibility method in SubtitleView, which seemed like a logical extension of subtitle styling functionality:

subtitleLayout = new SubtitleView(context);

public void setSubtitleStyle(SubtitleStyle style) {
    subtitleLayout.setUserDefaultStyle();
    subtitleLayout.setUserDefaultTextSize();

    if (style.getFontSize() > 0) {
        subtitleLayout.setFixedTextSize(TypedValue.COMPLEX_UNIT_SP, style.getFontSize());
    }
    subtitleLayout.setPadding(style.getPaddingLeft(), style.getPaddingTop(), style.getPaddingRight(), style.getPaddingBottom());
    subtitleLayout.setVisibility(style.getShowSubtitles() ? View.VISIBLE : View.GONE);
}

This approach was inspired by the CSS display property, which controls visibility in a somewhat similar fashion. However, I recognize that the analogy isn't perfect, as detailed in the display docs.

Addressing the selectedTextTrack={'disabled'} Suggestion

Implementing:

  selectedTextTrack={{
    type: SelectedTrackType.DISABLED,
  }}

indeed hides subtitles, aligning with the desired behavior across both Android and iOS platforms. However, this method also prevents the execution of events related to subtitle data changes, such as onCues for Android and legibleOutput for iOS, which are crucial for handling dynamic subtitle content.

Renaming showSubtitles to opacity

Regarding the suggestion to rename showSubtitles to opacity, I understand the rationale behind seeking a more semantically accurate term. However, the core functionality intended by showSubtitles is to toggle the visibility to either View.GONE or View.VISIBLE, which affects not just the opacity but also the layout space occupied by the subtitles. The term "opacity" might not fully capture the essence of this behavior, given its conventional association with visual transparency rather than layout impact.

Android Visibility Documentation Reference:

VISIBLE: The view is visible.
INVISIBLE: The view is invisible but still takes up space for layout purposes.
GONE: The view is invisible and does not take any space for layout purposes.

After considering the feedback and reflecting on the most straightforward way to implement this feature across both Android and iOS platforms, I believe maintaining the showSubtitles flag as a simple boolean (true/false) offers the most direct and developer-friendly approach.

The initial intent behind showSubtitles was to provide a clear, binary control over subtitle visibility, inspired by similar properties in web development like CSS's display.

For iOS, the equivalent behavior I believe can be achieved by setting suppressesPlayerRendering based on the showSubtitles value, allowing for event handling while controlling visibility:

let legibleOutput = AVPlayerItemLegibleOutput()
legibleOutput.suppressesPlayerRendering = !showSubtitles // Based on consumer prop

I'm open to further discussions and ready to adjust as needed to align with the project's standards and expectations. Your guidance is greatly appreciated, and I'm looking forward to finding the best path forward together. Thank you 🙏🏽.

[coofzilla]

You need also to fix conflict please !

Fixed the conflict :)

[coofzilla]

@freeboub

I ran the linter locally and didn't encounter any issues, does the action just need to be re-run? I don't believe there is a code-change in this PR that should cause the command not to be found? 🤔

[KrzysztofMoch]

You should run just yarn eslint ., does it not do anything ?
nvm removed cache and rerun worked

[coofzilla]

Going to resolve these new conflicts, then remove the subtitleStyle changes so we can get onCues and onTextTrackDataChanged merged.

[coofzilla]

@freeboub

New conflicts resolved, removed the showSubtitles flag under styles, and re-tested in example :)

[freeboub]

Don't you think adding following safety check would be a good idea ?

Suggested change

	if (!cueGroup.cues.isEmpty() && cueGroup.cues.get(0).text != null) {
	if (cueGroup != null && !cueGroup.cues != null && !cueGroup.cues.isEmpty() && cueGroup.cues.get(0).text != null) {

[coofzilla]

I totally agree with adding these safety checks; they’re a good idea. I think there might be a small typo in the suggestion, though. Shouldn’t !cueGroup.cues != null be cueGroup.cues != null?

if (cueGroup != null && cueGroup.cues != null && !cueGroup.cues.isEmpty() && cueGroup.cues.get(0).text != null) {

[freeboub]

I totally agree with adding these safety checks; they’re a good idea. I think there might be a small typo in the suggestion, though. Shouldn’t !cueGroup.cues != null be cueGroup.cues != null?

if (cueGroup != null && cueGroup.cues != null && !cueGroup.cues.isEmpty() && cueGroup.cues.get(0).text != null) {

And I am not sure it passes the linter 😅

[coofzilla]

When I tried doing these checks, the suggestion says to simplify to what I had originally? 🙈

And I am not sure it passes the linter 😅

which linter? I ran the check-android script and it had changes for an unrelated file; but, not this one?

How would you like me to proceed? 🫡

[coofzilla]

I didn't push these since these automated changes are unrelated to this PR.

[freeboub]

OK good, let's merge like this so !

[coofzilla]

sweet, that sounds awesome 🤩