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

Zoomable android #9

Merged
merged 7 commits into from
Aug 18, 2022
Merged

Zoomable android #9

merged 7 commits into from
Aug 18, 2022

Conversation

BlueBazze
Copy link
Contributor

Issue

Generally missing a zoom functionality.
Mentioned in:
apivideo/api.video-reactnative-live-stream#9

Description of the Change

Added a zoomRatio property which is a proxy for streamer.settings.camera.zoom.zoomRatio
Added a pinchGestureEnabled which will set setOnTouchListener on the preview (apiVideoView)
If pinchGestureEnabled is set to TRUE it will set setOnTouchListener to the pinchGesture which handles the pinch input and translates it to zoomRatio.
If pinchGestureEnabled is set to FALSE it will set setOnTouchListener to null meaning it is no longer using the pinchGesture

Both zoomRatio & pinchGestureEnabled should be public and can be set by the user of this package.

Possible Drawbacks

I am really still not sold by the structure of enabling and disabling the pinch gesture. pinchGestureEnabled would have to be set to true in the initialize, but if the user of the package dont want to enable it, they would have to disable it in their initialize.
And aswell the other way around, it its by default turned off.

Possible solution for this, could be if somehow we can call the set part of zoomRatio when the surface is created or something like that.
Another could be to take a initialFeatureConfig like the audio/videoConfig, and in that feature config we would have if the gestures should be enabled by default. This would call set on zoomRatio in the constructor.

From apivideo/api.video-swift-live-stream#8
Some people might get confused. With React Native, if you draw a View with Position: "Absolute" over the preview. That View would capture all of the click events. So you will need to add pointerEvents="none" to the view stretching over the preview to make touches go through that view.

Verification Process

Tested in development mode with the example app.

  1. Zoom in and stop the gesture
  2. Zoom in again and stop the gesture
  3. Zoom out and assert that the zoom does not do a quick jump when starting to zoom out.
  4. Zoom all the way out and asset that the zoom cant zoom further out than 1f ratio. (Meaning when you zoom in again it starts to zoom immediately)

Huawei P20 Lite (Android 9): was expected to fail as it has not received updates since android 9. It does not crash when trying to zoom. But it does crash when trying to enter the settings page.

Nokia T20 (Android 11): was expected to work seeing as it is running sdk 30. But it crashes immediately when zoomRatio is set.

Samsung A22 (Android 11 & 12): Worked well on both version 11 & 12 without any problems.
Tho the zooming itself felt slow. Pinching in and out (consistently in a slightly speeded up speed from what you normally would expect) for a little while can desync your gesture from the zoom happening on screen, as if the phone cant catch up.

Comment

I am only able to test on Android 12, since i have updated both Samsung A22 to Android 12. These are the only phones i have in office fit for testing this feature.
When setting the zoomRatio we should check for system sdk - and also check before enabling the pinch gesture.
This is a draft.

@BlueBazze
Copy link
Contributor Author

Restructured it a bit.
Moved pinchGestureEnabled into gesture config.
Added a double tap gesture to switch the camera facing.
Added a gestureConfig similar to audio/videoConfig. Which holds the enable/disable toggles for each gesture & specific gesture configs.

The GestureConfig:
I was a little uneasy with the namings of the properties. On one side, they could just be called what the gestures are (pinch, generic), but went for their functionality names (zoom, switchCamera) instead because of the generic gesture listener. It can also contain gestures for regular press, long press, fling & so on. So later if those are used a new gesture listener could be created for each gesture. Or just a single listener could be used.

Also, not sure if theres a better way to define the GestureConfig class & the sub classes for each gesture, in kotlin.

Tested the example app on:
Nokia T20 (Android 11), where the pinch gesture was not enabled due to the sdk version.
Samsung A22 (Android 12), where the pinch gesture was working as expected.

@BlueBazze BlueBazze marked this pull request as ready for review August 16, 2022 07:05
@ThibaultBee
Copy link
Member

ThibaultBee commented Aug 16, 2022

Hi,

Nice :)

Just sharing my reflexions at the moment:

  • As the View belongs to the user, I have the feeling that everything on the view should be implemented in the view part (ie the user application). Because :
  1. we don't have to handle stuff like zoom factor that sounds like specific behavior
  2. if the user also uses setOnTouchListener on his side and it breaks the zoom and the double tap.
  • I thought you were only interested by the zoom feature. Is there a reason why you also added switch camera on double tap? I am asking because switch on double tap kind of look like a specific behaviour.

  • With StreamPack, you have to make sure that the zoomRatio is within availableRatioRange not sure what happens otherwise. (unlike with HaishinKit).

Wdyt?

@BlueBazze
Copy link
Contributor Author

I think you are right.
But also, this functionality out of the box wouldnt hurt the attraction of the packages.

Havent thought about the user setting setOnTouchListener, but it could be circumvented with setting the gestures as public. They can set the GestureConfig.enabled = false, and manually control it. Meaning they would be able to use the preexisting gestures, and also implement their own.

Taking a look at the different camera apps (Snapchat, TikTok & Instagram), double tap is the default gesture for switching the camera facing. So this is probably what most end users are used to.
Though it should be mentioned, all phones i have access to, the default camera apps does not switch facing on double tap.

Zoom with StreamPack, you can literally zoom out of the app (It crashes). Setting the zoom factor to a high number will make it zooom fast. Not sure if it is the speed of the zoom that crashes the app, or if it is the high zoom ratio.
With Haishinkit, i havent felt any max limit on the zoom, but it seems like that zoom factor is lowered the higher the zoom ratio.

Not sure if i have buthered the terminology yet, or mixed something up yet.

....

Of course, the more functionality that is being added. The more can go wrong, and has to be maintained.
I wanted the functionality for React Native. So if you want to move all this to the RN package then it does not really affect my use case.

The more i think about it, it does seem like, from what i have made it does seem like an enforcement of how the broadcaster should work. I do believe i am biased.
People developing native apps can figure out this functionality pretty quickly compared to React Native devs.

Depending on what you want, i can move it to the RN package. Double click is not necessary, it was easy, that is the only reason i added it.

@ThibaultBee
Copy link
Member

Hi,

The libraries will have a zoomRatio API but you canl copy/paste your codeonTouchListener from the libraries to the examples to demonstrate how to use it. That would be great 👍

With StreamPack, you will have to clamp zoom ratio with availableRatioRange unlike with HaishinKit.

About the switch camera on double tap, we will drop it for now. It is a specific behaviour that user might not need.

So the part about the onTouchListener is going to be in native part of the RN project.

If you grant me the access to your libraries. I can help :)

@BlueBazze
Copy link
Contributor Author

Invited you to the repositories. Gonna do Android first.

@ThibaultBee
Copy link
Member

I was trying to clamp value with Range in StreamPack, I have tested to set zoom outside of availableRatioRange, it does not crash... the picture is just realllllyy noisyyy.
Arg, now I am not sure if I should clamp in StreamPack :(

@ThibaultBee
Copy link
Member

Ok, for clamp in StreamPack, see ThibaultBee/StreamPack@f13a0e4

@BlueBazze
Copy link
Contributor Author

Everything should be removed again, but keeping the zoomRatio.
So, the clamp should be passed up when setting the zoom, right?

Btw, i added a sdk version check when setting zoomRatio.
Just noticed on your changes you linked that StreamPack also has the check
StreamPack
Build.VERSION.SDK_INT >= Build.VERSION_CODES.R

The one i did
Build.VERSION.SDK_INT > Build.VERSION_CODES.R

Running on android 11, sdk 30. When zooming the preview crashes. Not the app.
The only error is
E/CameraDeviceCallback: Camera 0 is in error 4

@ThibaultBee
Copy link
Member

ThibaultBee commented Aug 17, 2022

Also added support Zoom on device with Android < R (see ThibaultBee/StreamPack@67c735e)

So with the future version of StreamPack (to be released):

  • You don't have to clamp the zoom value (already done by StreamPack)
  • You don't have to check Build.VERSION.SDK_INT > Build.VERSION_CODES.R or Build.VERSION.SDK_INT >= Build.VERSION_CODES.R

And it might be useful to return the supported zoom range. Right?

Don't worry, I will handle this.

@BlueBazze
Copy link
Contributor Author

Cool, ill adjust it to your remarks.

Yes that would be useful to expose the supported zoom range. But in that case. Couldnt it be useful to make streamer public? So users can access that directly?

@BlueBazze
Copy link
Contributor Author

Alright, everything should be ready both here. Not sure if you wanted me to do anything else from your last message.

Ill start on React Native now.

@ThibaultBee
Copy link
Member

Cool, ill adjust it to your remarks.

Yes that would be useful to expose the supported zoom range. But in that case. Couldnt it be useful to make streamer public? So users can access that directly?

api.video android live stream is a wrapper around a RTMP live streaming client. The purpose is to simplify the usage of a RTMP live streaming client. Moreover, we are using StreamPack as a RTMP live streaming client but we might want to change it if it is not good enough or if we find a better RTMP client. So, we don't want to expose RTMP client specific API.

I will make small changes on your commit. You don't have to do anything!

@BlueBazze
Copy link
Contributor Author

Alright, understood.

@ThibaultBee
Copy link
Member

Perfect 👍 Great job! Thank you :)
Sorry, I misguided you at the beginning. The setOnTouchListener seemed to be a nice idea!

I will merge after StreamPack release.
Do you have anything to add?

@BlueBazze
Copy link
Contributor Author

Dont worry, we got there at the end.

I dont have anything to add, it looks good.

The iOS pr is ready with the same changes as we have here. ZoomRatio & example zoom.
Gonna finish the draft for the RN pr today.

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

2 participants