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 763065 - androidmedia: assorted refactoring
androidmedia: assorted refactoring
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
git master
Other Linux
: Normal enhancement
: 1.9.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2016-03-03 22:56 UTC by Martin Kelly
Modified: 2016-03-24 12:58 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
0001-ahc-typo-fix.patch (741 bytes, patch)
2016-03-03 22:56 UTC, Martin Kelly
committed Details | Review
0002-ahc-remove-unneeded-include-stdio.h.patch (742 bytes, patch)
2016-03-03 22:57 UTC, Martin Kelly
committed Details | Review
0003-ahc-correct-error-message.patch (1006 bytes, patch)
2016-03-03 22:58 UTC, Martin Kelly
committed Details | Review
0004-amc-properly-deinit-when-ahcsrc-register-fails.patch (1.70 KB, patch)
2016-03-03 22:58 UTC, Martin Kelly
committed Details | Review
0005-ahc-use-gst-new-object-functions.patch (3.57 KB, patch)
2016-03-03 22:59 UTC, Martin Kelly
none Details | Review
0006-ahc-use-gst-unref-functions.patch (53.23 KB, patch)
2016-03-03 22:59 UTC, Martin Kelly
none Details | Review
0007-amc-move-Dalvik-VM-macros-into-common-header.patch (3.43 KB, patch)
2016-03-03 22:59 UTC, Martin Kelly
none Details | Review
[v2] 0005-ahc-use-gst-new-object-functions.patch (3.81 KB, patch)
2016-03-04 17:55 UTC, Martin Kelly
committed Details | Review
[v2] 0006-ahc-use-gst-unref-functions.patch (51.23 KB, patch)
2016-03-04 17:57 UTC, Martin Kelly
committed Details | Review
[v2] 0007-ahc-eliminate-AHC-_CALL-macros.patch (41.60 KB, patch)
2016-03-08 17:36 UTC, Martin Kelly
none Details | Review
[v3] 0001-ahc-eliminate-AHC-_CALL-macros.patch (43.30 KB, patch)
2016-03-09 18:35 UTC, Martin Kelly
committed Details | Review

Description Martin Kelly 2016-03-03 22:56:46 UTC
Created attachment 323032 [details] [review]
0001-ahc-typo-fix.patch

Attached are a few patches for refactoring inside androidmedia, particularly ahc. The patches are meant to be applied in series (in the order listed from the filenames), and I have tested them when applied all together.
Comment 1 Martin Kelly 2016-03-03 22:57:08 UTC
Created attachment 323033 [details] [review]
0002-ahc-remove-unneeded-include-stdio.h.patch
Comment 2 Martin Kelly 2016-03-03 22:58:20 UTC
Created attachment 323034 [details] [review]
0003-ahc-correct-error-message.patch
Comment 3 Martin Kelly 2016-03-03 22:58:50 UTC
Created attachment 323035 [details] [review]
0004-amc-properly-deinit-when-ahcsrc-register-fails.patch
Comment 4 Martin Kelly 2016-03-03 22:59:05 UTC
Created attachment 323036 [details] [review]
0005-ahc-use-gst-new-object-functions.patch
Comment 5 Martin Kelly 2016-03-03 22:59:21 UTC
Created attachment 323037 [details] [review]
0006-ahc-use-gst-unref-functions.patch
Comment 6 Martin Kelly 2016-03-03 22:59:36 UTC
Created attachment 323038 [details] [review]
0007-amc-move-Dalvik-VM-macros-into-common-header.patch
Comment 7 Martin Kelly 2016-03-03 23:00:56 UTC
I can split these into separate bugs if desired; I combined them together just to avoid the possibility of merge conflicts.
Comment 8 Sebastian Dröge (slomo) 2016-03-04 07:38:40 UTC
Review of attachment 323036 [details] [review]:

::: sys/androidmedia/gst-android-hardware-camera.c
@@ +2223,3 @@
         org_freedesktop_gstreamer_androidmedia_gstahccallback.constructor,
         *((jlong *) & cb), *((jlong *) & user_data));
+    if (err)

You're leaking the GError here (and probably want to use its content for logging at least?)

@@ +2260,3 @@
       android_hardware_camera_camerainfo.klass,
       android_hardware_camera_camerainfo.constructor);
+  if (err)

Same

@@ +2413,3 @@
         org_freedesktop_gstreamer_androidmedia_gstahccallback.constructor,
         *((jlong *) & cb), *((jlong *) & user_data));
+    if (err)

Same

@@ +2443,3 @@
         org_freedesktop_gstreamer_androidmedia_gstahccallback.constructor,
         *((jlong *) & cb), *((jlong *) & user_data));
+    if (err)

Same
Comment 9 Sebastian Dröge (slomo) 2016-03-04 07:41:30 UTC
Review of attachment 323037 [details] [review]:

::: sys/androidmedia/gst-android-hardware-camera.c
@@ +742,3 @@
     android_hardware_camera_parameters.WHITE_BALANCE_DAYLIGHT =
+        gst_amc_jni_object_ref (env, local);
+    gst_amc_jni_object_local_unref (env, local);

gst_amc_jni_object_make_global() does these two things at once
Comment 10 Sebastian Dröge (slomo) 2016-03-04 07:43:35 UTC
Review of attachment 323038 [details] [review]:

There already is gst_amc_jni_call_##_name##_method() and gst_amc_jni_call_static_##_name##_method()
Comment 11 Martin Kelly 2016-03-04 17:55:59 UTC
Created attachment 323114 [details] [review]
[v2] 0005-ahc-use-gst-new-object-functions.patch
Comment 12 Martin Kelly 2016-03-04 17:57:13 UTC
Created attachment 323115 [details] [review]
[v2] 0006-ahc-use-gst-unref-functions.patch
Comment 13 Martin Kelly 2016-03-04 18:00:32 UTC
Good points, thanks. I have submitted revised versions of 0005 and 0006.

As for patch 0007 (handling of GST_DVM_*_CALL), I agree that the macros are not necessary. However, they do help reduce the boilerplate code, as they are leveraged by the AHC_*_CALL macros. Would you prefer to entirely eliminate both macros and just use the gst_amc_jni_*_call_##_name##_methods ? If so, I can submit a patch for that.
Comment 14 Sebastian Dröge (slomo) 2016-03-04 22:54:45 UTC
I think so, or is there any advantages of the macros over the functions?
Comment 15 Martin Kelly 2016-03-05 00:04:31 UTC
The macros make for slightly shorter calls because they wrap up some of the error handling boilerplate and pass in the correct class for you (in the below example, android.hardware.Camera):

AHC_CALL (goto done, Void, autoFocus, object);

as opposed to:

gst_amc_jni_call_void_method (env, &err, object,
    android_hardware_camera.autoFocus);
if (err)
  goto done;

That said, there's enough boilerplate involved in handling Android JNI already that I'm not sure that the macro is worth it; it makes the code less standardized, and you have to define a separate macro for every Java class you might want to use. This opens up the potential for bugs if you accidentally the macro for a different class, which will turn into a runtime error in the JNI environment (!).

I think I'm in favor of replacing the macro with the standard function. If there are no objections, I'll go ahead and do that.
Comment 16 Sebastian Dröge (slomo) 2016-03-05 07:42:17 UTC
Go ahead :)
Comment 17 Martin Kelly 2016-03-08 17:36:17 UTC
Created attachment 323420 [details] [review]
[v2] 0007-ahc-eliminate-AHC-_CALL-macros.patch
Comment 18 Martin Kelly 2016-03-08 17:37:04 UTC
OK, I've attached a patch attached to remove all the AHC*_CALL macros. After writing it, I have mixed feelings about this patch. On one hand, the patch results in about 400 lines of extra code, which is unfortunate. On the other hand, it makes our Java calling conventions more standardized and provides better error handling and logging. Overall, I think I'm in favor of this change, but I'm curious what you think.

By the way, I checked the patch by a combination of code verification (as carefully as I could), running a camera pipeline, and checking for obvious signs of distress in adb. However, given the amount of code change here and the fact that a lot of the affected code paths involve error cases, I'm not sure I can test much more extensively. This is the kind of thing where we may just need to let it stabilize in the tree for a while.
Comment 19 Sebastian Dröge (slomo) 2016-03-09 07:12:37 UTC
Review of attachment 323420 [details] [review]:

Looks good to me except for one detail. It's more code but also given much more information, that's going to help a lot with debugging problems later.

::: sys/androidmedia/gst-android-hardware-camera.c
@@ +2181,3 @@
+    android_hardware_camera.addCallbackBuffer, buffer);
+  if (err) {
+    GST_ERROR ("Failed to call android.hardware.camera.addCallbackBuffer");

You probably want to include err->message in here... and everywhere else :) We should print the information we have
Comment 20 Martin Kelly 2016-03-09 18:35:11 UTC
Created attachment 323535 [details] [review]
[v3] 0001-ahc-eliminate-AHC-_CALL-macros.patch
Comment 21 Martin Kelly 2016-03-09 18:35:27 UTC
Good idea; new patch attached that includes err->message.
Comment 22 Sebastian Dröge (slomo) 2016-03-24 12:57:46 UTC
commit c25782921324cb9ff7bfcc757257dfde05f95df3
Author: Martin Kelly <martin@surround.io>
Date:   Mon Mar 7 17:23:23 2016 -0800

    ahc: eliminate AHC*_CALL macros
    
    Currently, we use AHC*_CALL macros to call many of the Camera functions.
    However, we already have helper classes to call the Camera functions, so
    eliminate the macros.
    
    As a nice side-benefit, we also get improved error handling and
    reporting when something goes wrong calling these functions, because a
    GError gets populated, and we log a GST_ERROR when something fails. This
    was harder to do using macros, as all error handling was hidden from the
    caller.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=763065

commit a2f270b3161a75185d5d9be76249f9b9fbbd9934
Author: Martin Kelly <martin@surround.io>
Date:   Thu Feb 18 11:29:06 2016 -0800

    ahc: use gst unref functions
    
    https://bugzilla.gnome.org/show_bug.cgi?id=763065

commit 1bfd6526d4518f1d60a044d8ff2f4611aefc5834
Author: Martin Kelly <martin@surround.io>
Date:   Thu Feb 18 14:08:13 2016 -0800

    ahc: use gst new object functions
    
    https://bugzilla.gnome.org/show_bug.cgi?id=763065