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 capture method #3577

Closed

Conversation

YangJonghun
Copy link
Collaborator

@YangJonghun YangJonghun commented Mar 10, 2024

Summary

Implement capture method

Motivation

Actually this feature(save image file) doesn't related player feature, but it is so much hard to implement within JS side or other native module. so I create this PR.
(If it doesn't fit your development plan, please close it.)

Changes

add method named capture

Test plan

just call await videoRef.current.capture() without below props

  • useSurfaceView
  • useSecureView
  • drm

and check below setup

@YangJonghun YangJonghun changed the title feat: add capture method feat: implement capture method Mar 10, 2024
@@ -68,6 +68,7 @@ class RCTVideo: UIView, RCTVideoPlayerViewControllerDelegate, RCTPlayerObserverH
private var _presentingViewController: UIViewController?
private var _pictureInPictureEnabled = false
private var _startPosition: Float64 = -1
private var _playerOutput: AVPlayerItemVideoOutput?
Copy link
Member

Choose a reason for hiding this comment

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

Does it increase memory usage significantly ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As I know, it depends on resolution or bitrate

@YangJonghun YangJonghun force-pushed the feat/capture-method branch from 8c5e4da to a7feb7e Compare March 11, 2024 16:50
@@ -265,6 +266,13 @@ const Video = forwardRef<VideoRef, ReactVideoProps>(
return VideoManager.setPlayerPauseState(false, getReactTag(nativeRef));
}, []);

const capture = useCallback(() => {
if (drm) {
throw Error('"capture" method can not be called with "drm" prop');
Copy link
Collaborator

Choose a reason for hiding this comment

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

throwing an exception is a bit violent ? console.warn should be enough ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

And BTW, you may defined an empty object in drm... I think you can just remove the test and handle it correctly in native code !


<PlatformsList types={['iOS', 'Android']} />

`capture(): Promise<void>`
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 possible to manage the generate file here ?
2 solutions:

  • allows to give a path / file path in parameters
  • give the generated file path in promise result

Because, here, I cannot know where the file is generated

import android.view.SurfaceView
import android.view.TextureView
import android.view.View
import androidx.media3.common.util.Util
Copy link
Collaborator

Choose a reason for hiding this comment

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

This import should not be in common folder : import androidx.media3.common.util.Util
Any way to remove it ?

@YangJonghun
Copy link
Collaborator Author

YangJonghun commented Mar 23, 2024

@freeboub @KrzysztofMoch
On second thought, I don't think it's necessary for this feature to be in this library.
I'd like to close this PR if you don't mind, as there is a memory growth issue, so feel free to close it.
(However, if someone asks for a capture feature, I'd like to point them to this PR so they can use it).

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.

3 participants