[Bug 788696] ahc2src: Introduce a new source for Android Camera 2 NDK API

[GStreamer (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=788696

Olivier Crête <olivier.crete at ocrete.ca> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #361162|none                        |needs-work
             status|                            |

--- Comment #6 from Olivier Crête <olivier.crete at ocrete.ca> ---
Review of attachment 361162:
 --> (https://bugzilla.gnome.org/review?bug=788696&attachment=361162)

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

-- 
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.