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 737786 - android: Upstream one of the android video source implementations
android: Upstream one of the android video source implementations
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
git master
Other Linux
: Normal enhancement
: 1.7.2
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks: 755173
 
 
Reported: 2014-10-02 15:11 UTC by Nicolas Dufresne (ndufresne)
Modified: 2016-01-21 19:08 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
porting from 0.10 to 1.0 (141.83 KB, patch)
2015-09-17 16:51 UTC, Justin Kim
none Details | Review

Description Nicolas Dufresne (ndufresne) 2014-10-02 15:11:19 UTC
As of today, two Android video source for GStreamer are available outside the main tree. I'd like to see one of these two (or a mix of both) merge into -bad.

From Ericsson Research OpenWebRTC (1.0):
https://github.com/EricssonResearch/openwebrtc-gst-plugins/tree/master/sys/androidvideo

From Collabora (not yet ported, 0.10):
http://cgit.collabora.com/git/user/tester/gst-plugins-bad.git/tree/sys/androidcamera?h=android-camerasrc-0.10

The first is much simplier, and support 1.0, while the second is a bit more complete, more error handling etc. Both injects a Java class to do callback handling. The second abstract the JNI logic to inject this with the goal to make it easier if we want more element being called back by the Java side.
Comment 1 Olivier Crête 2014-10-02 20:09:04 UTC
We also have a test app to go with the 0.10 source we built: http://cgit.collabora.com/git/user/tester/gst-camera-test.git
Comment 2 Sebastian Dröge (slomo) 2015-02-16 07:39:24 UTC
The build system of the Ericsson one (and how the Java foo is integrated) looks simpler. Also it's probably less bitrotten :)

Maybe we should move that one, and then add anything on top of it that the 0.10 one has additionally? And then at a later point looking at abstracting the Java handling into its own library, like the 0.10 one does. The library as is in the 0.10 one is not really useful anymore as the original code from androidmedia (where libdvm comes from) has changed quite a bit since then.
Comment 3 Nicolas Dufresne (ndufresne) 2015-02-16 13:59:52 UTC
(In reply to Sebastian Dröge (slomo) from comment #2)
> Maybe we should move that one, and then add anything on top of it that the
> 0.10 one has additionally? And then at a later point looking at abstracting
> the Java handling into its own library, like the 0.10 one does. The library
> as is in the 0.10 one is not really useful anymore as the original code from
> androidmedia (where libdvm comes from) has changed quite a bit since then.

Looks like a good approach. What interest me the most from the 0.10 work, is the configurability and the error handling. Re-implementing that on top or the "simpler" 1.0 version seems the most productive (and does not have to happen right away).
Comment 4 Olivier Crête 2015-02-16 15:29:02 UTC
We should probably use the gresource compiler instead of xxd, to avoid the extra dependency. It probably makes sense to package the generated files, so people can build the plugin without having the SDK installed.
Comment 5 Sebastian Dröge (slomo) 2015-02-16 15:39:52 UTC
(In reply to Olivier Crête from comment #4)
> We should probably use the gresource compiler instead of xxd, to avoid the
> extra dependency.

Only problem here is that we then unconditionally get GIO as a dependency, which is huge. But otherwise makes sense to me, just needs someone to do that ;)

> It probably makes sense to package the generated files, so
> people can build the plugin without having the SDK installed.

What do you mean? In cerbero we would ship the static plugin, which would then also include the Java resources and everything.
Comment 6 Olivier Crête 2015-02-16 15:53:17 UTC
We also unconditionally link with gio for anything that uses sockets anyway. 

I meant in cerbero, we'll need to add the sdk (not just the ndk) unless the pre-compiled class/dex/jar file are included, but it's more simple to include the .c/.h files like we do for orc.
Comment 7 Justin Kim 2015-08-12 06:35:07 UTC
Before taking advantage from both implementations, I am making collabora's code work with gst 1.x.

https://git.collabora.com/cgit/user/joykim/gst-plugins-bad.git/log/?h=ahc-wip

I just reuse jniutils in androidmedia to remove dependency on dvm, and the ported codes are placed in andromedia directory naturally. If andromedia is not a good place for the codes, jniutils should be a shared library like dvm did.
Can anybody advise me?
Comment 8 Sebastian Dröge (slomo) 2015-08-12 07:27:55 UTC
I was also just going to port the Collabora one to 1.x and use the JNI bits from androidmedia (and if necessary extend them). But if you already almost finished that... :)

Make sure that each commit on its own is compilable, so you probably have to squash all those per-file commits together. And the current solution for the Java files is to use the plugin-java-file magic from cerbero. Basically you place the .java files in some directory in share/gst-android (check the cerbero/data/ndk-build code for details).
Comment 9 Sebastian Dröge (slomo) 2015-08-12 07:29:06 UTC
Also try to keep history, the code at http://cgit.collabora.com/git/user/tester/gst-plugins-bad.git/tree/sys/androidcamera?h=android-camerasrc-0.10 has a clean history that we can just take over :) That will make it easier later to understand some decisions that were taken as the commit that added the code is in the history with its commit message.
Comment 10 Sebastian Dröge (slomo) 2015-08-27 08:05:17 UTC
Are you planning to update your branch?
Comment 11 Justin Kim 2015-08-27 09:01:35 UTC
If you are asking me, yes and I hope. but currently I stucked in crash with my wip branch. (and even I am not sure if it is only one critical issue ;) anyway, I need help to find what I did wrong if my work is worth it.

I ported android application also
https://git.collabora.com/cgit/user/joykim/gst-camera-test.git/

With my wip branch, a crash on gl stuff happens. but I think it's not gl issue.
It might be wrong somewhere my ported code, maybe GstBuffer handling in ahcsrc/on_preview_frame function. Here's back trace. 

Program received signal SIGSEGV, Segmentation fault.
[Switching to Thread 19963]
0x400de810 in __memcpy_base () from /Users/justin/Documents/git/gst-camera-test/obj/local/armeabi/libc.so
(gdb) bt
  • #0 __memcpy_base
    from /Users/justin/Documents/git/gst-camera-test/obj/local/armeabi/libc.so
  • #1 gst_gl_base_buffer_upload_cpu_write
    at gstglbasebuffer.c line 269
  • #2 _default_map_buffer
    at gstglbasebuffer.c line 284
  • #3 _gl_mem_map_buffer
    at gstglmemory.c line 857
  • #4 _map_data_gl
    at gstglbasebuffer.c line 335
  • #5 _run_message_sync
    at gstglwindow.c line 638
  • #6 _run_message_async
    at gstglwindow.c line 707
  • #7 g_main_context_invoke_full
    at gmain.c line 5714
  • #8 g_main_context_invoke
    at gmain.c line 5675
  • #9 gst_gl_window_default_send_message
    at gstglwindow.c line 657
  • #10 gst_gl_context_thread_add
    at gstglcontext.c line 1632
  • #11 _mem_map_full
    at gstglbasebuffer.c line 363
  • #12 gst_memory_map
  • #13 _do_convert_one_view
    at gstglcolorconvert.c line 1585
  • #14 _do_convert
    at gstglcolorconvert.c line 1737
  • #15 _run_message_sync
    at gstglwindow.c line 638
  • #16 _run_message_async
    at gstglwindow.c line 707
  • #17 g_main_dispatch
    at gmain.c line 3210
  • #18 g_main_context_dispatch
    at gmain.c line 3874
  • #19 g_main_context_iterate
    at gmain.c line 3945
  • #20 g_main_loop_run
    at gmain.c line 4139
  • #21 gst_gl_context_create_thread
    at gstglcontext.c line 1363
  • #22 g_thread_proxy
    at gthread.c line 764
  • #23 __thread_entry
    from /Users/justin/Documents/git/gst-camera-test/obj/local/armeabi/libc.so
  • #24 pthread_create
    from /Users/justin/Documents/git/gst-camera-test/obj/local/armeabi/libc.so
  • #25 ??

Comment 12 Sebastian Dröge (slomo) 2015-08-27 09:10:04 UTC
This crash doesn't come from your code it seems, but from the GL thread. What is the test application doing? Just outputting the camera to glimagesink?

Does outputting something else (videotestsrc) to glimagesink work for you? Does the camera work if you use fakesink instead of glimagesink? Does the camera work if you use something like
  ... ! "video/x-raw,format=AYUV" ! fakesink
as the sink (to check if videoconvert likes your buffers or also crashes)?
Comment 13 Justin Kim 2015-08-27 09:16:58 UTC
> What is the test application doing? Just outputting the camera to glimagesink?
Yes, it's very simple

These two cases just worked well.

videotestsrc !  video/x-raw,format=(string)NV21 ! glimagesink name=vsink sync=false

ahcsrc !  video/x-raw,format=(string)NV21 ! fakesink name=vsink sync=false


but, only this causes crash
 
ahcsrc !  video/x-raw,format=(string)NV21 ! glimagesink name=vsink sync=false


> (to check if videoconvert likes your buffers or also crashes)?
I didn't check with videoconvert yet. I will.
Comment 14 Justin Kim 2015-08-27 09:33:14 UTC
With this pipeline, the point moves to videoconvert

ahcsrc !  video/x-raw,format=(string)YV12 ! queue ! videoconvert ! video/x-raw,format=(string)NV21  ! glimagesink name=vsink sync=false


Program received signal SIGSEGV, Segmentation fault.
[Switching to Thread 5924]
_backup_video_orc_unpack_I420 (ex=<optimized out>) at tmp-orc.c:942
942         var37 = ptr4[i];
(gdb) bt
  • #0 _backup_video_orc_unpack_I420
    at tmp-orc.c line 942
  • #1 video_orc_unpack_I420
    at tmp-orc.c line 1024
  • #2 do_unpack_lines
    at video-converter.c line 2505
  • #3 gst_line_cache_get_lines
    at video-converter.c line 418
  • #4 video_converter_generic
    at video-converter.c line 2826
  • #5 gst_video_convert_transform_frame
    at gstvideoconvert.c line 692
  • #6 gst_video_filter_transform
    at gstvideofilter.c line 271
  • #7 default_generate_output
    at gstbasetransform.c line 2166
  • #8 gst_base_transform_chain
    at gstbasetransform.c line 2319
  • #9 gst_pad_chain_data_unchecked
    at gstpad.c line 4085
  • #10 gst_pad_push_data
    at gstpad.c line 4337
  • #11 gst_pad_push
    at gstpad.c line 4453
  • #12 gst_queue_push_one
    at gstqueue.c line 1336
  • #13 gst_queue_loop
    at gstqueue.c line 1483
  • #14 gst_task_func
    at gsttask.c line 331
  • #15 g_thread_pool_thread_proxy
    at gthreadpool.c line 307
  • #16 g_thread_proxy
    at gthread.c line 764
  • #17 __thread_entry
    from /Users/justin/Documents/git/gst-camera-test/obj/local/armeabi/libc.so
  • #18 pthread_create
    from /Users/justin/Documents/git/gst-camera-test/obj/local/armeabi/libc.so
  • #19 ??

Comment 15 Sebastian Dröge (slomo) 2015-08-27 09:46:41 UTC
Ok, so whatever buffers/memory you provide from your source is broken then in one way or another. It could be something simple as stride/plane offset mismatches, or also that the memory became freed/invalid before downstream used it (which should be prevented with the various GstMemory mechanisms on your side).
Comment 16 Justin Kim 2015-09-15 07:14:58 UTC
I fixed a crash problem on my branch so this works now. However, handling java file is still vague a bit.

Gstreamer.java is managed in cerbero and this file is placed in project directory when ndk-build run. but for this android camera, Callback java class is located in gst-plugins-bad and even it should be built before linking with android application.

so I am a bit confused about what solution is more proper for initial upstream android camera. Could you advise me?
Comment 17 Sebastian Dröge (slomo) 2015-09-15 07:24:49 UTC
You can install your .java file in share/gst-android/ndk-build/$plugin/ and then the GStreamer ndk-build integration will build it into the application.

This is from memory though, check the code in gstreamer-1.0.mk. There should be something like GSTREAMER_PLUGINS_CLASSES in there, which is supposed to automatically do this.
Comment 18 Sebastian Dröge (slomo) 2015-09-15 07:26:46 UTC
Also please preserve the GIT history of the 0.10 camera code, it contains many hints at why things were done like this. And ideally squash some of your own commits together so that we have something compilable with each commit.
Comment 19 Justin Kim 2015-09-17 16:47:36 UTC
The plugin-java-file magic from cerbero works very well and it seems to solve troublesome java class issue. :)

I finished to cherry-pick the history of 0.10 works.
But as you know, it makes huge conflicts so there's no other option except for using ours strategy.

$ git cherry-pick -X ours ...

You can see that the entire commits are on the top of master branch in here;

https://git.collabora.com/cgit/user/joykim/gst-plugins-bad.git/log/?h=ahc-history-preserved

you might want to get the commits from '51d5b4f androidmedia: Add androidmedia plugin' to HEAD in the url above. the HEAD commit which I made is to restore build.
Comment 20 Justin Kim 2015-09-17 16:51:45 UTC
Created attachment 311570 [details] [review]
porting from 0.10 to 1.0

This patch is created on the top of ahc-history-preserved branch, 'https://git.collabora.com/cgit/user/joykim/gst-plugins-bad.git/log/?h=ahc-history-preserved'

Unfortunately, current cerbero recipe doesn't deploy java file. I'll attach cerbero patch also in different issue.
Comment 21 Sebastian Dröge (slomo) 2015-10-14 14:06:15 UTC
Just for the record, the Collabora one will be merged some time soon and not the Ericsson one. It has more features, is more well tested and integrates with the existing JNI helpers we have.
Comment 22 Sebastian Dröge (slomo) 2015-10-25 08:38:13 UTC
That the commits touch the audio/video encoders/decoders and you revert most of the changes later is not good. When preserving the history, you should try to filter changes only to the relevant files (that is, the new files) and keep everything else untouched.

Or we could just have the old 0.10 stuff in a branch, that is then merged into master. That was master always stays buildable at least. Nonetheless don't merge any changes to existing files just to revert these changes again later.

Or we really just ignore the history, mention it in the commit message (where to find the history, where the stuff comes from) and just do a single commit with the final result.

All messy :)

Opinions?
Comment 23 Justin Kim 2015-10-25 14:03:47 UTC
(In reply to Sebastian Dröge (slomo) from comment #22)
> That the commits touch the audio/video encoders/decoders and you revert most
> of the changes later is not good. When preserving the history, you should
> try to filter changes only to the relevant files (that is, the new files)
> and keep everything else untouched.
> 

I agree, but it's not easy to filter changes because some of changes have dependency with major history a lot. To do like that, imo, more comments should be required between history to explain what or how the changes are modified. 

> Opinions?

How about mixing your 2nd and 3rd opinion together?
Having another 0.10 branch for android camera, then in the master branch, we can mention the branch. :)
Comment 24 Tim-Philipp Müller 2015-10-25 16:09:16 UTC
I think we should have history in a branch that's then merged into master, that's also how I do plugin moves these days.

You can filter out things with git filter branch, so that e.g. the branch only has those files you want to keep and nothing else (and commits that touch other files simply have the bits that don't touch files we're interested in removed).
Comment 25 George Kiagiadakis 2015-10-29 17:54:37 UTC
Hi,
I want to use this branch and for ease of integration with cerbero, I picked and squashed the entire ahc-history-preserved branch into a single commit, on top of which I also squashed https://git.collabora.com/cgit/user/joykim/gst-plugins-bad.git/commit/?h=ahc&id=9ff3c982c13ef51d0fc5393ec8bd73438f25802c (which seems to be the same patch as the one attached here)

Squashing and patching works fine, and the result is quite clean (no references to anything outside androidmedia), but it looks like the examples are broken. First of all, there are 2 of them, very similar to each other; not sure what is the difference. Second, they contain references to eglglessink as well as GST_X_OVERLAY...
Comment 26 Justin Kim 2015-10-30 02:37:06 UTC
I am not sure if I understand how/what I should do for this.

@Tim, your plan is to create a branch in for history preserving, then will merge a squashed commit on top of master. do I correctly understand?

@George, thank you for testing.
> but it looks like the examples are broken.
Which example was broken? Do you mean https://git.collabora.com/cgit/user/joykim/gst-camera-test.git/ ?
Comment 27 George Kiagiadakis 2015-10-30 11:10:00 UTC
(In reply to Justin J. Kim from comment #26)
> I am not sure if I understand how/what I should do for this.
> 
> @Tim, your plan is to create a branch in for history preserving, then will
> merge a squashed commit on top of master. do I correctly understand?

As I understand it, the plan is to create a branch starting from somewhere in the 0.10 series that contains all the history of the android plugin, without any touches in other files (using git filter-branch to filter what the commits contain) and then merge this in master (no squashing, just git merge). This avoids rebasing the entire history on top of master, which I think makes sense for this case, because otherwise master would contain non-compilable commits.

> @George, thank you for testing.
> > but it looks like the examples are broken.
> Which example was broken? Do you mean
> https://git.collabora.com/cgit/user/joykim/gst-camera-test.git/ ?

No, the example from this repository is good, but the *same* example also exists in the ahc-history-preserved branch (together with one more very similar example) and is broken there.
Comment 28 Justin Kim 2015-12-31 15:09:31 UTC
Hello, Happy new year!

https://git.collabora.com/cgit/user/joykim/gst-plugins-bad.git/log/?h=filterout-ahc

I made a branch again to filter out files which are not interested in preserving history. When I tested, it works well except one thing.

glimagesink has longer delays (about 1~2 min after building pipeline). This delay issue is reproduced when I used 'videotestsrc'.

Please check again the url. Thank you!
Comment 29 Sebastian Dröge (slomo) 2015-12-31 15:25:25 UTC
Is the problem with glimagesink maybe fixed by http://cgit.freedesktop.org/gstreamer/gst-plugins-bad/commit/?id=22a0914cce9f6352e3042b9f1f7b9cacac2db0b7 ? Or were you using that version already?
Comment 30 Justin Kim 2016-01-01 09:24:14 UTC
(In reply to Sebastian Dröge (slomo) from comment #29)
> Is the problem with glimagesink maybe fixed by
> http://cgit.freedesktop.org/gstreamer/gst-plugins-bad/commit/
> ?id=22a0914cce9f6352e3042b9f1f7b9cacac2db0b7 ? Or were you using that
> version already?

Thank you for information. Although I didn't check only one commit unit which you point out, it works fine after rebasing to yesterday HEAD (http://cgit.freedesktop.org/gstreamer/gst-plugins-bad/commit/?id=1e5eb72532613efb15bb687df68b6fb5048896c0)
Comment 31 Nicolas Dufresne (ndufresne) 2016-01-08 19:44:55 UTC
I was about to merge this, but found that latency reporting is wrong. It reports min/max base on min/max frame duration. Max latency is how long can the stream be delayed before the information get lost by the source. Also, the largest frame duration makes a lot of more sense for min latency. Try it out, remove sync=0 to the test program and try it in a darker room. You endup with a slideshow.
Comment 32 Tim-Philipp Müller 2016-01-08 19:59:43 UTC
I was looking the at the git foo the other day, I had something slightly different in mind, but guess this would work too.
Comment 33 Nicolas Dufresne (ndufresne) 2016-01-08 20:32:22 UTC
Ok, the timestamping code is also completely random (it timestamp buffer with the previous buffer timestamp and duration, and drop the first one). I would complete drop that code, and let the base class pick a timestamp, leave the duration to unknown.
Comment 34 Nicolas Dufresne (ndufresne) 2016-01-08 21:51:44 UTC
Ok, sorry for the noise, but the timestamp code do one thing right, it timestamp as soon as possible (in the Android callback). Doing timestamp at ->create() time does not look nice here.
Comment 35 Nicolas Dufresne (ndufresne) 2016-01-11 17:16:37 UTC
Ok, I think it's good enough to merge now. To preserve history and not mess with bisect, we need a non fast-foward merge to happen. Though, this is disabled on our repository. Tim and/or Sebastian will take care as soon as they find the time. Here's the final branch link with a fix for the latency:

https://git.collabora.com/cgit/user/nicolas/gst-plugins-bad.git/log/?h=android-hardware-camera

And my master, with the merge commit:

https://git.collabora.com/cgit/user/nicolas/gst-plugins-bad.git/log/
Comment 36 George Kiagiadakis 2016-01-11 22:38:26 UTC
I'm sorry, but as I've said before, the example in this branch is outdated. It still references GstXOverlay from 0.10...

https://git.collabora.com/cgit/user/nicolas/gst-plugins-bad.git/tree/tests/examples/androidcamera/gst-camera-test/jni/camera_test.c?h=android-hardware-camera#n7
Comment 37 Tim-Philipp Müller 2016-01-11 22:49:59 UTC
I think the examples need more work anyway, there are also some namespacing issues that need to be fixed. I think we should merge the camerasrc itself first, and the examples can always be added back later as a single commit, I don't think history is so interesting for the example code.
Comment 38 Nicolas Dufresne (ndufresne) 2016-01-11 23:18:34 UTC
Agreed, do you have time to filter this out or shall I ?
Comment 39 Tim-Philipp Müller 2016-01-11 23:27:07 UTC
I can, but not before tomorrow evening at the earliest, so feel free to do it yourself :)
Comment 40 Sebastian Dröge (slomo) 2016-01-18 09:11:27 UTC
What's the status here?
Comment 41 Tim-Philipp Müller 2016-01-18 09:22:40 UTC
I think it's waiting for me to fix up the git stuff and push it (because of the merge commit the server-side hooks will have to be disabled for that).
Comment 42 Nicolas Dufresne (ndufresne) 2016-01-18 15:37:42 UTC
And we agreed to remove the patches that adds the example (they are broken apparently, I only tested with the example repo provided). That should not be hard though.
Comment 43 Nicolas Dufresne (ndufresne) 2016-01-21 19:08:16 UTC
So, that should make things simpler from now on. For any issues with this code, please open specific bugs. I'll open one to put back an example in -bad.

commit 2193fa8ffeca7830cdce195c6cbff4cb16717178
Merge: 5b04e77 09dbc5b
Author: Nicolas Dufresne <nicolas.dufresne@collabora.com>
Date:   Thu Jan 21 13:50:44 2016 -0500

    Merge branch 'android-hardware-camera'
    
    This branch adds support for Android Hardware Camera API through a new
    element called ahcsrc. This is the "old" Android Camera API, then only
    API available on Android 4.X.
    
    https://bugzilla.gnome.org/show_buf.cgi?id=737786