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

VIDCS-869: Fix crash when toggling video #462

Merged
merged 3 commits into from
May 25, 2023
Merged

Conversation

vona-ben
Copy link
Collaborator

@vona-ben vona-ben commented May 16, 2023

What is this PR doing?

Fixes an issue where crashing may occur when toggling video

How should this be manually tested?

To reproduce, follow the steps detailed in #461

To confirm fix, checkout this branch and then follow the same steps as before 🎉

@vona-ben vona-ben changed the title Update video captuer Update basic video2captuer May 16, 2023
@vona-ben vona-ben changed the title Update basic video2captuer Update Basic Video2Catpurer May 16, 2023
@vona-ben vona-ben requested a review from v-kpheng May 16, 2023 07:31
@v-kpheng v-kpheng changed the title Update Basic Video2Catpurer VIDCS-869: Update Basic Video2Catpurer May 18, 2023
@juliobecerragomez
Copy link

juliobecerragomez commented May 18, 2023

Has this been updated based on our default camera2 capturer?

Comment on lines 457 to 466
/*
executeAfterCameraOpened = () -> {
cameraState = CameraState.CLOSING;
if (camera != null) {
camera.close();
}
};

*/
executeAfterCameraOpened = null;

Choose a reason for hiding this comment

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

If I remember well, this code should not be commented out, and the null assignment removed.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the same. Not sure why this got commented out. May be we can check the history when this change was made.

Copy link
Contributor

@goncalocostamendes goncalocostamendes left a comment

Choose a reason for hiding this comment

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

Left one question

@vona-ben
Copy link
Collaborator Author

vona-ben commented May 23, 2023

@juliobecerragomez @goncalocostamendes Those changes have been taken from the native SDK code. I haven't made any modifications other than renaming variables and fixing the logs. I think @jintgeorge would be best to reply to the comment here (cc @v-kpheng )

@goncalocostamendes
Copy link
Contributor

@juliobecerragomez @goncalocostamendes Those changes have been taken from the native SDK code. I haven't made any modifications other than renaming variables and fixing the logs. I think @jintgeorge would be best to reply to the comment here (cc @v-kpheng )

Yes, just confirmed it is the same as native SDK. Approving

Copy link
Contributor

@goncalocostamendes goncalocostamendes left a comment

Choose a reason for hiding this comment

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

LGTM!

@juliobecerragomez
Copy link

@vona-ben So at least we should fix the commented out code. I do not think it is ok to show it as part of the linux sample apps.

@vona-ben
Copy link
Collaborator Author

@vona-ben So at least we should fix the commented out code. I do not think it is ok to show it as part of the linux sample apps.

Done

Copy link
Contributor

@goncalocostamendes goncalocostamendes left a comment

Choose a reason for hiding this comment

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

+1

Copy link
Contributor

@jintgeorge jintgeorge left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link

@juliobecerragomez juliobecerragomez left a comment

Choose a reason for hiding this comment

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

LGTM!

@v-kpheng v-kpheng changed the title VIDCS-869: Update Basic Video2Catpurer VIDCS-869: Fix crash when toggling video May 24, 2023
@vona-ben vona-ben merged commit f39e385 into main May 25, 2023
28 of 29 checks passed
@vona-ben vona-ben deleted the update-video-captuer branch May 25, 2023 02:51
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.

None yet

4 participants