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: implement startPosition #3355

Merged

Conversation

YangJonghun
Copy link
Collaborator

@YangJonghun YangJonghun commented Nov 16, 2023

Update the changelog

  • feat: implement startPosition prop (defined within source prop)
  • (Breaking Change) source.startTime have been renamed to source.cropStart
  • (Breaking Change) source.endTime have been renamed to source.cropEnd

Provide an example of how to test the change

  • this change should test with source.cropStart
    • If the source.cropStart prop is applied, it will be applied from that point forward.
  • this change should test with SSAI

Describe the changes

### `contentStartTime`
The start time in ms for SSAI content. This determines at what time to load the video info like resolutions. Use this only when you have SSAI stream where ads resolution is not the same as content resolution.

Platforms: Android, iOS
Platforms: Android
Copy link
Collaborator

Choose a reason for hiding this comment

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

good catch !

@freeboub
Copy link
Collaborator

Point 1:

this change should test with source.startTime
--> should not be an issue, but the startPosition will be applied from startTime
Not sure this is clean, source.startTime / endTime are cutting the video so source.startTime become position 0. This position will be the base for applied startPosition.

this change should test with SSAI: -> I agree

Point 2:

Can I propose you to move this property inside the source object ?
Why am I asking that ?

  • What is the expected behavior if we just change startPosition without changing ? -> should it restart the stream with startPosition or should it just seek ?
  • do it have sense to change source and keep current startPosition ? (I think no)

the background of this request is also the real time issue your are pointing in the media3 PR. the postDelayed is necessary to ensure all props are well applied. If we correctly want to remove this delay when starting the stream, all props necessary to start the playback shall be stored in the same property.

Let me know if this is not clear !

(else the code looks good to me!)

@YangJonghun
Copy link
Collaborator Author

YangJonghun commented Nov 16, 2023

should not be an issue, but the startPosition will be applied from startTime

I agree (I'll make sure to include that in the documentation as well)

this change should test with SSAI: -> I agree

Unfortunately, I don't have any sample to test

Can I propose you to move this property inside the source object ?

I deeply agree (I think DRM should be included for the same reason (all props necessary to start the playback shall be stored in the same property.))

I agree with all your points, but I think startTime and startPosition can be confusing because of their names (as an aside, I think contentStartTime is also unintuitive).

So, while it might be a breaking change, I was wondering what you think of the following modification.

source={{
  uri: "..."
  startPosition: 0,
  crop: {
    startTime: 0,
    endTime: 0
  }
}}

@freeboub
Copy link
Collaborator

freeboub commented Nov 16, 2023

You are right...
For ads sample, It should be easy to test with the basic sample!
Regarding to naming, I agree this is confusing.
I can propose you cropStart and cropEnd, it can be an easier change. (be carefull you need to warn this is a breaking API change ! ) but your proposal works for me also !

I deeply agree (I think DRM should be included for the same reason (all props necessary to start the playback shall be stored in the same property.))

Yes ! to be follow up in: #2693

#### Start playback at a specific point in time

Provide an optional `startPosition` for video. Value is in milliseconds. If the `cropStart` prop is applied, it will be applied from that point forward.
(If it is zero or negative, it is ignored)
Copy link
Collaborator

Choose a reason for hiding this comment

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

It should not be ignored if set to 0 !

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You are right! I'll fix it

Copy link
Collaborator

@freeboub freeboub left a comment

Choose a reason for hiding this comment

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

We need to handle 0 as a seek position.
Some video (like live streams) which starts to the end of playback by default.
I think this prop should be also applied in that case.

To be exact, on my product there is a "restart" option on live stream.
It take the EPG and create a kind of VOD with it. This VOD has a close start (always start at the same point), but an open end (video chuck are added according to live updates).

In taht case, exoplayer consider this video as a live stream, thus it starts the video at the end, where, when customer client on restart, he wants video to start from the beggining.

(And thank you for the rework of the prop)

ios/Video/RCTVideo.swift Outdated Show resolved Hide resolved
"time": NSNumber(value: _startPosition),
"tolerance": NSNumber(value: 100)
])
_startPosition = -1
Copy link
Collaborator

Choose a reason for hiding this comment

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

is it really need to force _startPosition to -1 ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I was assuming that the playback would work fine, then an error would occur, and then it would work again.

When the source is unchanged and the lifecycle is as follows,

(1) readyToPlay → (2) failed → (3) readyToPlay

I forced it to -1 because I would expect startPosition to not work in state (3).

But I'm not very experienced with iOS, and I don't know much about it. If my assumptions are wrong, feel free to let me know and I'll correct above code.

docs/pages/component/props.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@KrzysztofMoch KrzysztofMoch left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@KrzysztofMoch KrzysztofMoch merged commit 2648502 into TheWidlarzGroup:master Nov 24, 2023
1 check passed
@YangJonghun YangJonghun deleted the feat/implement-start-position branch November 24, 2023 15:00
@ihopeyouwin
Copy link

ihopeyouwin commented Jan 24, 2024

I am wondering what if we do videofile imports through require, and still need to use cropstart/end feature, in that case we cannot directly get access to uri of that file. Maybe it will be better if you consider making it more flexible?

@freeboub
Copy link
Collaborator

I am wandering what if we do videofile imports through require, and still need to use cropstart/end feature, in that case we cannot directly get access to uri of that file. Maybe it will be better if you consider making it more flexible?

You are right! I think we should allow (or move ) the require usage inside uri prop of the source!

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.

Implement startPosition prop to set the start time of a video
4 participants