Skip to content

Capture Frame Feature - #1

Draft
kevinvangelder wants to merge 3 commits into
mainfrom
capture-frame
Draft

Capture Frame Feature#1
kevinvangelder wants to merge 3 commits into
mainfrom
capture-frame

Conversation

@kevinvangelder

Copy link
Copy Markdown

No description provided.

@joshdhenry joshdhenry left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code looks good. Going to test it now with the example app.

Comment thread ios/RCTTWVideoModule.m
}

- (void)captureLocalFrame:()string {
if (self.camera) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Nit: Might be best to fail early instead of nesting this block of code.

if (!self.camera) {
   return;
}

@joshdhenry

joshdhenry commented Apr 13, 2023

Copy link
Copy Markdown

Running into errors trying to run the example app. I don't think its related to this code though. Just a haste module naming collision. Will see if I can get past it.

Comment thread ios/RCTTWVideoModule.m
}

// Start the capture session
[captureSession startRunning];

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Will this work even though the camera is already being used for video capture?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I do not know, I didn't get far enough to start testing it.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@joshdhenry I saw you approved this PR. Were you able to get the example app running and test it out?

}

private void captureFrame(Promise promise) {
cameraCapturer.onFrameCaptured(VideoFrame frame);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This code doesn't look like it would compile

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

That is quite likely, I was early in piecing together code from various sources when I switched to working on the web implementation.

dfinn added a commit that referenced this pull request Aug 30, 2023
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