After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 788696 - ahc2src: Introduce a new source for Android Camera 2 NDK API
ahc2src: Introduce a new source for Android Camera 2 NDK API
Status: RESOLVED OBSOLETE
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
git master
Other All
: Normal enhancement
: git master
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2017-10-09 06:22 UTC by Justin Kim
Modified: 2018-11-03 14:14 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
ahc2src: new source element for camera2ndk on Android (138.49 KB, patch)
2017-10-09 06:22 UTC, Justin Kim
none Details | Review
cebero: add configs to support Android Nougat (1.43 KB, patch)
2017-10-09 06:31 UTC, Justin Kim
none Details | Review
ahc2src: new source element for camera2ndk on Android (138.49 KB, patch)
2017-10-09 06:36 UTC, Justin Kim
none Details | Review
ahc2src: new source element for camera2ndk on Android (74.89 KB, patch)
2017-10-09 06:47 UTC, Justin Kim
none Details | Review
RFC: ahc2src for android camera HAL3 (127.58 KB, patch)
2018-05-09 03:12 UTC, Justin Kim
none Details | Review

Description Justin Kim 2017-10-09 06:22:29 UTC
Created attachment 361159 [details] [review]
ahc2src: new source element for camera2ndk on Android

Since Android Nougat, Android provides Camera 2 NDK APIs so no JNI wrappers are required to implement ahc2src. Therefore, cebero's 'target_distro_version' should be 'DistroVersion.ANDROID_NOUGAT' or greater to build this element.

One outstanding result of this element is the quick response to discarding internal buffers so 763308 will not happen.
Comment 1 Justin Kim 2017-10-09 06:31:42 UTC
Created attachment 361160 [details] [review]
cebero: add configs to support Android Nougat

I am not sure this change is required for cebero master branch, but I added it just for reference.
Comment 2 Justin Kim 2017-10-09 06:36:13 UTC
Created attachment 361161 [details] [review]
ahc2src: new source element for camera2ndk on Android
Comment 3 Justin Kim 2017-10-09 06:47:38 UTC
Created attachment 361162 [details] [review]
ahc2src: new source element for camera2ndk on Android

Removed unnecessary diffs.
Comment 4 Olivier Crête 2017-11-08 21:45:00 UTC
Review of attachment 361160 [details] [review]:

I wonder if we can build this element at this level, but while building the rest with the older level. What happens if you don't set that flag ?
Comment 5 Justin Kim 2017-11-09 08:02:06 UTC
(In reply to Olivier Crête from comment #4)
> Review of attachment 361160 [details] [review] [review]:
> 
> I wonder if we can build this element at this level, but while building the
> rest with the older level. What happens if you don't set that flag ?

Do you mean what happens without "target_distro_version = DistroVersion.ANDROID_NOUGAT"? I haven't tried but I don't think I can manage it because some of headers are different among android platform versions.

That's why I added checking condition to `configure.ac` not to build if camera2ndk library is missing. So "target_distro_version" is not set as Nougat or greater version, 'ahc2src' will not be built.
Comment 6 Olivier Crête 2017-11-13 20:26:27 UTC
Review of attachment 361162 [details] [review]:

::: sys/androidmedia/gstahc2src.c
@@ +119,3 @@
+    gst_object_unref (self->ahc2src);
+    g_clear_pointer (&self->image, (GDestroyNotify) AImage_delete);
+  }

Shouldn't this function also free the "self" pointer if the refcount is 0?

Any reason not to use g_atomic_int_inc() & g_atomic_int_dec_and_test() for this boxed type?

@@ +123,3 @@
+
+G_DEFINE_BOXED_TYPE (GstWrappedAImage, gst_wrapped_aimage,
+    gst_wrapped_aimage_ref, gst_wrapped_aimage_unref)

Why did you create a boxed type for this? Isn't a free function enough if you only use it through gst_buffer_new_wrapped() ? Or do you want to expose the AImage itself to another element?

@@ +132,3 @@
+G_DEFINE_TYPE_WITH_CODE (GstAHC2Src, gst_ahc2_src, GST_TYPE_PUSH_SRC,
+    G_IMPLEMENT_INTERFACE (GST_TYPE_PHOTOGRAPHY, gst_ahc2_src_photography_init)
+    G_ADD_PRIVATE (GstAHC2Src)

You don't need a private separate from the main struct. This is a plugin and the header is not installed, so everything is private.

@@ +212,3 @@
+  }
+
+  g_clear_pointer (&metadata, (GDestroyNotify) ACameraMetadata_free);

This is a stack pointer, you can just do ACameraMetadata_free(metadata); directly without the atomic operation inside g_clear_pointer()

@@ +1183,3 @@
+  GST_DEBUG_OBJECT (self, "Starting capture request");
+  ACameraCaptureSession_setRepeatingRequest (priv->camera_capture_session,
+      NULL, 1, &priv->capture_request, NULL);

Doing repeating requests is a good start. But I think we should also be able to support the full capabilities of the API, including bursts. See my talk of the GStreamer conference for a plan on how to do this.. That said, this plan probably also means porting it away from GstBaseSrc

@@ +1699,3 @@
+
+  if ((clock = GST_ELEMENT_CLOCK (self))) {
+    GstClockTime base_time = GST_ELEMENT_CAST (self)->base_time;

You want to use gst_element_get_clock() + gst_element_get_base_time() which take the lock.

@@ +1732,3 @@
+    AImage_delete (image);
+    GST_DEBUG_OBJECT (self, "Droping image (reason: %s)",
+        priv->started ? "first frame" : "not yet started");

I'm not sure we need to drop the first image. You can just put a duration of GST_CLOCK_TIME_NONE. The frames don't really have a duration at all.

@@ +1876,3 @@
+  properties[PROP_CAMERA_TEMPLATE_TYPE] =
+      g_param_spec_int ("camera-template-type", "Camera Template Type",
+      "A request template for the camera running purpose", -1, G_MAXINT,

Instead of an int here, you probably want to expose an enum with the numbers from the Android API.
Comment 7 Olivier Crête 2017-11-13 20:28:46 UTC
Review of attachment 361162 [details] [review]:

Also, if this is not using the JNI stuff at all, I think it should be a separate plugin from androidmedia... Maybe androidndk ?
Comment 8 Justin Kim 2018-05-09 03:12:06 UTC
Created attachment 371828 [details] [review]
RFC: ahc2src for android camera HAL3

This patch is still work in progress, but I'd like to get reviewed and ask some questions which I can't find a solution yet.
Comment 9 Justin Kim 2018-05-09 03:23:57 UTC
Review of attachment 371828 [details] [review]:

Here's my commit history.

https://gitlab.collabora.com/joykim/gst-plugins-bad/commits/wip/ndk-camera2

::: sys/androidndk/gstahc2src.c
@@ +315,3 @@
+
+  /* Just sending a repeat-request currently */
+  ndk_capture_session_set_request (self->capture_session,

This is my unsolved problem. The 'capture_session' should be one instance (if another session is created, the previous one is closed by Android),
but this source element is able to have multiple 'capture_request' if request pads are created.

If I set 'capture_session' and restart repeat request, it influences on the stream through always pad (blinking).

I think this is not a way to use Android camera properly, but I can't find any good example to send another request while streaming.
Comment 10 GStreamer system administrator 2018-11-03 14:14:15 UTC
-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/gstreamer/gst-plugins-bad/issues/618.