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.

Created attachment 361161 [details] [review]
ahc2src: new source element for camera2ndk on Android

Created attachment 361162 [details] [review]
ahc2src: new source element for camera2ndk on Android

Removed unnecessary diffs.

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 ?

(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.

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.

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 ?

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.

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.

-- 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.