GNOME Bugzilla – Bug 763065
androidmedia: assorted refactoring
Last modified: 2016-03-24 12:58:32 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.
Created attachment 323033 [details] [review] 0002-ahc-remove-unneeded-include-stdio.h.patch
Created attachment 323034 [details] [review] 0003-ahc-correct-error-message.patch
Created attachment 323035 [details] [review] 0004-amc-properly-deinit-when-ahcsrc-register-fails.patch
Created attachment 323036 [details] [review] 0005-ahc-use-gst-new-object-functions.patch
Created attachment 323037 [details] [review] 0006-ahc-use-gst-unref-functions.patch
Created attachment 323038 [details] [review] 0007-amc-move-Dalvik-VM-macros-into-common-header.patch
I can split these into separate bugs if desired; I combined them together just to avoid the possibility of merge conflicts.
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
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
Review of attachment 323038 [details] [review]: There already is gst_amc_jni_call_##_name##_method() and gst_amc_jni_call_static_##_name##_method()
Created attachment 323114 [details] [review] [v2] 0005-ahc-use-gst-new-object-functions.patch
Created attachment 323115 [details] [review] [v2] 0006-ahc-use-gst-unref-functions.patch
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.
I think so, or is there any advantages of the macros over the functions?
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.
Go ahead :)
Created attachment 323420 [details] [review] [v2] 0007-ahc-eliminate-AHC-_CALL-macros.patch
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.
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
Created attachment 323535 [details] [review] [v3] 0001-ahc-eliminate-AHC-_CALL-macros.patch
Good idea; new patch attached that includes err->message.
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