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

Fix various issues with NativeCamera/VideoTexture #1107

Open
12 of 16 tasks
ryantrem opened this issue Jul 20, 2022 · 2 comments
Open
12 of 16 tasks

Fix various issues with NativeCamera/VideoTexture #1107

ryantrem opened this issue Jul 20, 2022 · 2 comments
Assignees
Milestone

Comments

@ryantrem
Copy link
Member

ryantrem commented Jul 20, 2022

An initial implementation of NativeCamera and VideoTexture was merged mid 2021 (for Android and iOS), but these implementations have some issues that need to be addressed for these components be ready for production usage:

Changes Needed

Android

  • Resolution selection - currently, it seems NativeCamera on Android gets the highest resolution camera feed, creates a texture that is exactly sized to the min width/height requested, and then uses a shader to draw the full res camera texture into the newly created texture. Instead, I believe we need to be querying the camera to find a resolution that most closely satisfies the constraints (min width/height), and create a second (renderable) texture that is the same size, and then draw the source texture into the target texture. NOTE: It seems like there might be a useful example for resolution/frame rate enumeration for Camera2 here: https://github.com/android/camera-samples/tree/main/Camera2Video. It is a Java example, but should map the the native API closely.
    • It looks to me like we can enumerate resolutions like this:
      ACameraMetadata_const_entry configInfo{}; ACameraMetadata_getConstEntry(metadataObj, ACAMERA_SCALER_AVAILABLE_STREAM_CONFIGURATIONS, &configInfo); for (int32_t i = 0; i < configInfo.count; i += 4) { const int32_t width{entry.data.i32[i + 1]}; const int32_t height{entry.data.i32[i + 2]}; }
    • From https://developer.android.com/ndk/reference/group/camera#acameradevice_createcapturesession, it looks to me like we can select a resolution by calling m_surfaceTexture.setDefaultBufferSize (needs to be added to JavaWrappers) and setting the resolution before creating the Surface from the SurfaceTexture.
  • Orientation - from some quick testing, it seems like the VideoTexture orientation/rotation is not the same as in the browser. We should take a closer look and see if the shader should be doing some additional rotation or something.
  • API Level - the NDK Camera2 API is used, which requires API level 24. The code currently does a compile time check, but really we want to do a runtime check and return an error or something so that the package as a whole can still be used with lower API levels (and NativeCamera will fail gracefully).
  • Calling Pause twice on the video object causes an exception as it tries to release the camera session that has already been released. Pause in Native video should likely no-op when the video is already "paused". Additionally the NativeCameraImpl should be resilient enough to also handle having the Close method called twice.

iOS

  • Resolution selection - currently, NativeCamera on iOS gets the highest resolution camera feed and uses the texture directly (because it is renderable directly). Again we should instead be looking for the closest native camera resolution that meets the constraints.
  • Format - the BJS side creates the texture and specifies the format (as RGBA8, in NativeEngine.createDynamicTexture), and then the native side overrides the texture handle in the Graphics::Texture, but the other properties (including format) are not updated, so the format (BGRA8) is incorrect, and operations like readPixels won't work. Probably we should convert the BGRA8 texture to RGBA8 in the iOS NativeVideo implementation (perhaps via a simple shader).
  • Orientation - from some quick testing, it seems like the VideoTexture orientation/rotation is not the same as in the browser.
  • Threading - camera capture and rendering happen on two different threads but share the same texture (camera writes to it, rendering reads from it). This seems to cause problems particularly with reading pixels from the VideoTexture, where one of three things happen: 1) The image is successfully captured, 2) The captured image is all black, 3) The app crashes in bgfx::frame

Shared

  • NativeVideo exposes a width/height that are just whatever was passed in for the min width/height. Today this is technically correct on Android, but will be incorrect when we actually select a resolution that just most closely matches the requested width/height. And today on iOS it is just wrong. These width/height properties of the NativeVideo eventually flow back into the Graphics::Texture, so they need to be correct for operations like readPixels to succeed for the texture. I think the right thing to do is probably pass the min width/height to NativeVideo (like to does today), and have NativeCameraImpl::Open return a struct that describes the actual video selected (e.g. the actual width/height etc.), and then have NativeVideo just set videoWidth/videoHeight properties on itself based on that result from the call to Open.
  • The texture size can also change when the device orientation changes (matching the browser behavior). In that case, the underlying native texture needs to be recreated, and the updated size needs to be communicated back to NativeVideo. This should probably happen by returning a size from NativeCameraImpl::UpdateCameraTexture (similar to NativeCameraImpl::Open). When NativeVideo calls NativeCameraImpl::UpdateCameraTexture and it returns a result with a different size, in addition to the videoWidth/videoHeight properties being updated, we should also raise the resize event (same as browser).
  • From a NAPI standpoint, NativeCamera and VideoTexture are too chatty (lots of back and forth between native and JS). We've seen this negatively impact perf in other scenarios, so it's probably true in this case too. We have a lot of instance accessors that just return a value and so should just be properties set directly on the object. For the easy ones we should probably just fix them, and it may also be worth doing a little profiling to see if we are spending too much time in napi c++ <==> js communication per frame.
  • OverrideTexture appears to only be implemented for Android and it's not very clear what it's usage is. We should consider either improving the documentation for that feature flag, or removing it if it's no longer used.
  • Constraints are not passed to the createTextureFromWebcam function in the same format as BabylonJS in the web context. For example in the web we expect max width to be specified in a nested width object to be passed { width: { max: 720}}, whereas in Babylon Native the arguments are passed in a flat object via the maxWidth property { maxWidth: 720, maxHeight: 0, facingMode: "environment" }
  • InternalTexture.updateSize (referenced elsewhere in this issue) calls Engine.updateTextureDimensions. It seems like if we properly implement that for Babylon Native, then perhaps we don't need to do the bgfx::overrideInternal, and instead could just use the underlying texture that has already been created. If we do the other resize fixes mentioned in this issue, then the existing underlying should already always be the correct dimensions when it comes time to copy the camera texture into the Babylon texture.

Babylon.js

  • In the browser, when the device's orientation changes (e.g. portrait to landscape), the browser application rotates (the browser as an app does not choose to lock the orientation) and the videoWidth/videoHeight of the underlying HTMLMediaElement swap. However, this is not communicated to the underlying InternalTexture, and as such VideoTexture.getSize() will return the wrong value. What we probably should be doing is registering for the resize event on HTMLMediaElement (right after video.addEventListener("playing", onPlaying);) and when the size changes, call this._texture.updateSize(...). Having the correct texture size is necessary if you are trying to create a plane for example that visualizes the video stream with the correct aspect ratio.
  • Dispose throws an exception when it calls VideoTexture::removeSource because the native video implementation does not implement the removeAttribute function.

Background

Here is a little background that may be helpful. At a high level, in the browser things start with a call to getUserMedia. A constraints object can be passed in as an arg, and can specify things like the requested width/height, either as a min, max, ideal, or exact value. getUserMedia is supposed to look at all these constraints and return the closest fit (this seems subjective and like there would just need to be some heuristics to decide, so I'm not sure if this is consistent across browsers). I imagine fully supporting all possible constraints in NativeCamera would be quite a task, so we can probably continue supporting just a subset (e.g. just min width/height and front facing vs. back facing camera, for example).

@CedricGuillemet
Copy link
Contributor

CedricGuillemet commented Jul 21, 2022

Texture management

On Android, only 1 RGBA texture is used for rendering and OES->RGBA conversion. I think it might result in a performance issue. The 1st context using it will block the other one until work is done. It would be great to do a profiling with and without 2 textures.

@bghgary
Copy link
Contributor

bghgary commented Aug 3, 2022

Related: #924

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

No branches or pull requests

4 participants