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

feat: Added setPlaybackRate and tracks #63

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

Conversation

plauzul
Copy link

@plauzul plauzul commented May 18, 2020

Platforms affected

Android

Motivation and Context

possibility of accelerating videos on android, and viewing subtitles with setMediaTracks

Description

Possibility to change the playbackrate of the video on android, and visualization of subtitles with setMediaTracks. If possible do the same on IOS, I am not IOS developer

Testing

emulator, real device and chromecast

Checklist

  • I've updated the documentation as necessary
  • If this Pull Request resolves an issue, I linked to the issue in the text above (and used the correct keyword to close issues using keywords)
  • [x ] I've run npm test and no errors were found (run npm style to auto-fix errors it can)
  • [ x] I've run the tests (See Readme)
  • I added automated test coverage as appropriate for this change (See Readme Contributing section)

@@ -32,7 +32,7 @@ NS_ASSUME_NONNULL_BEGIN
- (void)addMessageListener:(CDVInvokedUrlCommand*)command;
- (void)sendMessage:(CDVInvokedUrlCommand*) command;
- (void)mediaPlay:(CDVInvokedUrlCommand*)command;
- (void)mediaPause:(CDVInvokedUrlCommand*)command;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think mediaPause should be removed here?

/**
* Pauses the media item.
* @param {chrome.cast.media.PauseRequest} pauseRequest The optional media pause request.
* @param {function} successCallback Invoked on success.
* @param {function} \ successCallback Invoked on success.
Copy link
Collaborator

Choose a reason for hiding this comment

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

typo '/' here

@Lindsay-Needs-Sleep
Copy link
Collaborator

Lindsay-Needs-Sleep commented May 25, 2020

Hi @plauzul !

Thanks for submitting this! And following up with the iOS implementation! (I was just going to ask for it. :p)

I looked through your changes it all looks pretty good I think. (I haven't run it yet, really swamped right now. Hopefully I will be able to test it in a few weeks. T.T)

Regarding subtitles

I haven't worked with this before.
How can we test this?
(eg. if you load a video with valid tracks, does it update any parameter in the media object?)

Do the subtitles need to be embed in the video already, or do they come from a separate url?

Regarding setPlaybackRate

So there a couple things:

1)

Before I add it, I do really want a test to be added to the auto tests. (With the re-write I worked really hard vamping up the tests so that the plugin will be stable hopefully!) This test should be quite easy to write. (You can probably write it without actually running it. If the test has little problems, I won't mind fixing it when I get around to actually testing this.)

Two tests should be created:

  • it('media.setPlaybackRate should set playback rate to requested rate'
    • Should check that the media object's playbackRate is updated on success
  • it('media.setPlaybackRate should return error when media is finished'

They should go in the describe('Media (non-queues)' section of tests_auto.js.
They can probably go just before each equivalent media.seek test.

2)

Issue #64, Doesn't really affect this PR, but it might affect how this feature is accessed in the future.

3)

Kind of related to Issue #64, the only way I found for setting the playback rate from a desktop chrome browser was this:

session.sendMessage(
    "urn:x-cast:com.google.cast.media", 
    {
        mediaSessionId: session.media[0].mediaSessionId, 
        requestId: 12345, 
        type: 'SET_PLAYBACK_RATE',
        playbackRate: 2
    }, 
    function () {
        console.log('success');
    }, function (err) {
        console.log(err);
    });

This is kind of interesting because that might mean that the native-side implementation of setPlaybackRate might not be required.

  • I believe we have a sendMessage implementation in Android and probably iOS
  • But it is completely untested as far as I know. (Issue Does anyone use session.sendMessage? #65)
  • If it does work, we could just have media.setPlaybackRate call the function above. (Less code, which is great. But also a bit unfortunate that this info came late. xD)

If you have everything set up still, could you run the sendMessage code above and see if it works? I am very curious.

4)

Upon testing the sendMessage command in desktop chrome, I found that only playback rates of 0.5 - 2 were valid.
Is this the same for your implementation?

5)

The SetPlaybackRateRequestData for the sendMessage function can also take relativePlaybackRate instead of playbackRate. It doesn't seem as useful, but I thought I would just mention it incase you have some thoughts about it.

@Lindsay-Needs-Sleep Lindsay-Needs-Sleep mentioned this pull request Jun 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants