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

Camera: Added API to change avfoundation camera provider orientation #7263

Merged
merged 1 commit into from Dec 13, 2020

Conversation

misl6
Copy link
Member

@misl6 misl6 commented Dec 12, 2020

This PR adds the ability to programmatically change the orientation of the video feed provided by the AvFoundation camera provider.

Maintainer merge checklist

  • Title is descriptive/clear for inclusion in release notes.
  • Applied a Component: xxx label.
  • Applied the api-deprecation or api-break label.
  • Applied the release-highlight label to be highlighted in release notes.
  • Added to the milestone version it was merged into.
  • Unittests are included in PR.
  • Properly documented, including versionadded, versionchanged as needed.

@@ -626,7 +633,10 @@ -(int) updateImage {
if (image == NULL)
image = new CameraFrame((int)width, (int)height);

if (image->datasize != width * height * sizeof(char) * 4) {
bool resolution_change_detected = image->datasize != width * height * sizeof(char) * 4;
bool orientation_change_detected = (image->width == height) && (image->height == width);
Copy link
Member

Choose a reason for hiding this comment

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

What if the width and the height are the same?

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, in that case, We don't need to update the image properties, cause there's no need to re-create a Texture on the Kivy side.

The user will just see a flipped/rotated image accordingly to his selection.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm what I meant, unless I'm misreading it, when the width and height are the same, orientation_change_detected will be true and then the if will be executed for each frame. Meaning it will do a malloc and free on a every frame, which is not desired.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ahh, no you don't misread that, I was not thinking at that case at all (on iOS I should stick to one of the available presets, so I didn't had the opportunity to test this specific case).

But, on MacOS, the user can specify the width + height, so it's a good point, let me fix this one ASAP.

Copy link
Member

Choose a reason for hiding this comment

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

Looking at this again, I think we're trying to be too clever, when we should always update the width and height, but only update the buffer if the buffer size needs to change.

Setting the width and height on every frame doesn't cost much. And i think the original and current implementation is still buggy. E.g. what if width and height are 5, 4, respectively. And it changes to 2, 10. The width and height won't be updated. Maybe it never happens, but it's asking fot trouble.

Copy link
Member Author

Choose a reason for hiding this comment

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

What a cleaner and nicer alternative. Pushed and tested it.���

Yeah, unfortunately all the provider implementation it's asking for trouble, as I can see We have a multitude of issues on the avfoundation camera provider.

Sometimes also crashes for a BADACCESS, I'm trying to fix one thing per time, but it's going to be a long journey :)

@misl6 misl6 force-pushed the feature/avfoundation-camera-orientation branch from 520d2c9 to 88fc1fd Compare December 13, 2020 08:40
@misl6 misl6 force-pushed the feature/avfoundation-camera-orientation branch from 88fc1fd to e06ff58 Compare December 13, 2020 09:16
@matham matham added this to the 2.1.0 milestone Dec 13, 2020
@matham matham merged commit b5ec51e into kivy:master Dec 13, 2020
@matham
Copy link
Member

matham commented Dec 13, 2020

Yeah, unfortunately all the provider implementation it's asking for trouble, as I can see We have a multitude of issues on the avfoundation camera provider.

The only solution to these and other such issues is to have unittests. We can have a mock camera class that just serves images from a memory or a video file to test core camera or uix/camera. However, I don't see how that can help us test these providers. Perhaps there's a way to mock avfoundation camera and somehow test it? It would be worth investigating.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants