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

Can seek in milliseconds #1028

Closed
wants to merge 6 commits into from
Closed

Conversation

keenjoe007
Copy link

The seek method was able to seek only in seconds, so we could seek in milliseconds.

@cobarx
Copy link
Contributor

cobarx commented May 28, 2018

Thanks for submitting this. I'm a bit worried about merging this as precise seeking can result in delays while seeking. From the Apple docs:

Sample accurate seeking may incur additional decoding delay which can impact seeking performance.

I tested seeking with the current code and if I input a fraction like 35.5, the onSeek method shows I'm at exactly 35.5 when it completes.

Are you wanting to be able to specify seek times in milliseconds (e.g. 35500ms rather than 35.5s) or finding that the seeking is not accurate enough?

@keenjoe007
Copy link
Author

@cobarx
Thank you for your reply.

Seek in milliseconds looks correct in the onseek method, but it is incorrect in the actual video display.
In the project that I am using now, there is a certain value (dec. 3.539483956) up to the decimal point. In seek (3.539483956), the display of the actually displayed video was different from the expected result.

Therefore, I made this fix and sent it.

When using it, if you want to see, for example, 3.5 seconds, it is assumed not to use like seek (3500), but to use like seek (3.5).

I noticed this bug when stopping the video with the pause method and then seek.

I am not good English Speaker. Sorry for Unclear sentences...

@cobarx
Copy link
Contributor

cobarx commented May 29, 2018

@keenjoe007 Got it, I understand what you're saying. I guess it depends on how the video is encoded if the existing seek will go to the right spot.

What I'd suggest is we add an optional second argument to see for when you need an exact seek:
seek(position, toleranceInMS)
If tolerance is not set, it defaults to 1000 to maintain the existing behavior.

What do you think? This would provide exact seeking for people like you who need it, and keep the high performance version for people who don't. If you're happy with this approach would you mind updating the PR. I can take care of updating the docs.

@keenjoe007
Copy link
Author

@cobarx
Thanks!
I will upgrade my PR!

@cobarx
Copy link
Contributor

cobarx commented Jun 1, 2018

Great progress! Looking forward to getting this merged when it's all done.

@keenjoe007
Copy link
Author

@cobarx
There is something I want you to tell.

I attempted to divide the setSeek method in ios / RCTVideo.m by the number of arguments, but it did not work. For example as follows.

- (void) setCurrentTime: (float) currentTime
{
   [self setSeek: currentTime];
}

- (void) setSeek: (float) seekTime
{
   [self setSeek: seekTime toleranceInMS: 1000];
}

- (void) setSeek: (float) seekTime toleranceInMS: (int) toleranceInMS
{
   ... Conventional setSeek processing
}

The seek method to be called with javascript executed only - (void) setSeek: (float) seekTime method even if it callsseek (4.555664, 0). The second argument did not make sense.

How do I fix it? From javascript, seek (position, toleranceInMS) can you get the second argument like this?

@cobarx
Copy link
Contributor

cobarx commented Jun 5, 2018

@keenjoe007 I don't believe that React Native supports passing in more than one argument when setting a prop, so we'll need to use multiple props.

Looking at:
https://github.com/react-native-community/react-native-video/blob/master/ios/RCTVideo.m#L564
There is are pendingSeek and pendingSeekTime values. I think what we should do is add a third value for pendingSeekTolerance. To do that, change the JS side to:

  seek = (time, tolerance = 1000) => {
    this.setNativeProps({
       seek: time,
       seekTolerance: tolerance
    });
  };

and add the appropriate native prop at the bottom.

Now on the iOS side, when JS seek gets called, we need to make sure both seek & seekTolerance are set. So what you can do is create a setSeekTolerance function and whenever setSeek or setSeekTolerance is called, you set pendingSeek = true and pendingSeekTime or pendingSeekTolerance as appropriate. Once pendingSeek is true and both pendingSeekTime and pendingSeekTolerance are set, we apply the seek. To know if they have been set, they should have a default value like -1 since 0 is a valid seek time and tolerance.

Let me know if that makes sense or you need further help.

@keenjoe007
Copy link
Author

ummmmm...

@cobarx
Copy link
Contributor

cobarx commented Jun 11, 2018

What's the ummm for? Is it all ready?

@keenjoe007
Copy link
Author

Sorry, my local environment is broken yesterday .
Please wait....
Is the revision this directional?

@cobarx
Copy link
Contributor

cobarx commented Jun 13, 2018

I thought of an idea how to do this where we can pass everything in at once. I'll test it and send you sample code shortly.

@cobarx
Copy link
Contributor

cobarx commented Jun 13, 2018

Ok it works. Here are the changes needed to get the data passed in

in Video.js:

  seek = (time, tolerance = 1000) => {
    if (Platform.OS === 'ios') {
      this.setNativeProps({
        seek: { time, tolerance }
      });
    } else {
      this.setNativeProps({ seek: time });
    }
  };

also:

Video.propTypes = {
  /* Native only */
  src: PropTypes.object,
  seek: PropTypes.oneOfType([
    PropTypes.number,
    PropTypes.object
  ]),

in RCTVideoManager.m:

RCT_EXPORT_VIEW_PROPERTY(seek, NSDictionary);

in RCTVideo.m:

- (void)setSeek:(NSDictionary *)info
{
  float seekTime = [info[@"time"] floatValue];
  int tolerance = [info[@"tolerance"] intValue];

That should be enough to get the data you need. You'll need to update setCurrentTime and the setSeek functions.

@cobarx
Copy link
Contributor

cobarx commented Jun 21, 2018

@keenjoe007 Thanks for your work on this, I adapted it and got it working with the dictionary method. Since I don't have access to your repo, I created a new PR #1076. I appreciate you suggesting and getting this started.

@cobarx cobarx closed this Jun 21, 2018
@keenjoe007
Copy link
Author

Sorry...
I could not creating time for developing this issue.

Thanks for take over this issue and advise me about .
@cobarx

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.

2 participants