[sailfishos][webrtc] Enable support for WebRTC video. JB#53982 #3

d-grigorev · 2021-06-25T17:17:19Z

No description provided.

abranson · 2021-06-28T08:17:46Z

rpm/0070-sailfishos-webrtc-Implement-video-capture-module.-JB.patch

+namespace webrtc {
+namespace videocapturemodule {
+
+#define GST_DROID_CAMERA_PRIMARY   "primary"


Are these still valid? I think we have to use the camera number now

Yes, this is reported as camera ID, see GstDroidCameraManager::GetDeviceName() below. These names are later translated to native wording, see https://github.com/sailfishos/sailfish-components-webview/blob/7b47472eac71d31792bff2ee6766e03683a890e2/import/popups/WebrtcPermissionDialog.qml#L36

llewelld

I'm not able to do much more than a sanity check of the code I'm afraid. It broadly looks sensible and builds fine. I've made a few comments, but nothing major.

However, when I tried it on an XA2 with gmp-droid-0.2-1.2.1 I got some strange output when testing it with the meetecho Janus test server you recommended. I get the same results with both the front and back cameras, but I don't know why this is I'm afraid.

llewelld · 2021-07-02T07:42:52Z

rpm/0069-sailfishos-webrtc-Enable-GMP-for-encoding.-JB-53982.patch

+                      ipc::Shmem* aMem) = 0;
+   virtual void Dealloc(ipc::Shmem& aMem) = 0;
+
+  Mutex mMutex;


Could this be private?

I accidentally left it public. Thank you.

llewelld · 2021-07-02T07:48:36Z

rpm/0069-sailfishos-webrtc-Enable-GMP-for-encoding.-JB-53982.patch

+   // 3* is because we're using 3 buffers per frame for i420 data for now.
+-  if ((NumInUse(GMPSharedMem::kGMPFrameData) >
+-       3 * GMPSharedMem::kGMPBufLimit) ||
+  if ((NumInUse(GMPSharedMem::kGMPFrameData) > 3 * GMPSharedMem::kGMPBufLimit) ||


I think this is just an accidental formatting change. If so, it might be better to skip it from the patch.

Yes, I've been fiddling with these lines. This is a candidate for future revision. Will remove this from the patch.

llewelld · 2021-07-02T07:50:20Z

rpm/0069-sailfishos-webrtc-Enable-GMP-for-encoding.-JB-53982.patch

+-       3 * GMPSharedMem::kGMPBufLimit) ||
+  if ((NumInUse(GMPSharedMem::kGMPFrameData) > 3 * GMPSharedMem::kGMPBufLimit) ||
+       (NumInUse(GMPSharedMem::kGMPEncodedData) > GMPSharedMem::kGMPBufLimit)) {
+


Similarly here; the extra newline looks unintentional.

rpm/0069-sailfishos-webrtc-Enable-GMP-for-encoding.-JB-53982.patch

llewelld · 2021-07-02T14:44:24Z

rpm/0070-sailfishos-webrtc-Implement-video-capture-module.-JB.patch

+
+    if (deviceNameLength >= strlen(cameraName))
+    {
+        memcpy(deviceNameUTF8, cameraName, strlen(cameraName));


If deviceNameLength == strlen(cameraName) then this could leave deviceNameUTF8 without a null terminator.

Yeah. I copied this code from media/webrtc/trunk/webrtc/modules/video_capture/linux/ and edited just to fit my internal Camera interface. I never tested it and it is not listed in supported camera types. Will review that better.

llewelld · 2021-07-02T14:46:55Z

rpm/0070-sailfishos-webrtc-Implement-video-capture-module.-JB.patch

+        char* deviceUniqueIdUTF8,
+        uint32_t deviceUniqueIdUTF8Length,
+        char* productUniqueIdUTF8,
+        uint32_t productUniqueIdUTF8Length,


Does it matter that productUniqueIdUTF8 is never populated (in particular, is there any danger the caller will assume the return value will be null terminated)?

llewelld · 2021-07-02T14:48:21Z

rpm/0070-sailfishos-webrtc-Implement-video-capture-module.-JB.patch

+        {
+            memset(deviceUniqueIdUTF8, 0, deviceUniqueIdUTF8Length);
+            memcpy(deviceUniqueIdUTF8, cap.bus_info,
+                   strlen((const char*) cap.bus_info));


if deviceUniqueIdUTF8Length == strlen((const char*) cap.bus_info) this could leave the returned buffer without a null terminator I think.

llewelld · 2021-07-02T15:00:20Z

rpm/0070-sailfishos-webrtc-Implement-video-capture-module.-JB.patch

+    rtc::scoped_refptr<Camera> OpenCamera (const char* deviceUniqueIdUTF8);
+private:
+    int32_t FillCapabilities(int fd, std::vector<VideoCaptureCapability>& caps);
+    unsigned _numberOfDevices;


Is _numberOfDevices needed? It looks unused.

rpm/0070-sailfishos-webrtc-Implement-video-capture-module.-JB.patch

d-grigorev · 2021-07-02T17:20:20Z

I'm not able to do much more than a sanity check of the code I'm afraid. It broadly looks sensible and builds fine. I've made a few comments, but nothing major.

However, when I tried it on an XA2 with gmp-droid-0.2-1.2.1 I got some strange output when testing it with the meetecho Janus test server you recommended. I get the same results with both the front and back cameras, but I don't know why this is I'm afraid.

That's bad. Looks like the image format from viewfinder is totally different. I have no access to XA2 to check, and gecko is cut off the logging in production build. I will ask is someone has the device.

rpm/0070-sailfishos-webrtc-Implement-video-capture-module.-JB.patch

d-grigorev · 2021-08-07T08:57:54Z

Although I tested it on a sony device and it works, I found out that sony's video encoder doesn't support I420 pixel format input. The problem is in gmp-droid, a fix PR is coming soon. Unfortunately sony uses semi-planar format internally, which is not compatible with gecko's internals. Gecko has surprisingly bad design of video input, however I see some optimization opportunities here.
The only known working scenario is local echo, like https://mozilla.github.io/webrtc-landing/gum_test.html.

d-grigorev · 2021-08-07T09:07:39Z

This patch uses https://github.com/d-grigorev/amber-camera/tree/initial as a video input library. It contains two specs, a library and a plugin. The plugin uses droidmedia-devel at build time and droidmedia at runtime.

d-grigorev · 2021-08-09T16:58:16Z

The encoder issue is fixed in sailfishos/gmp-droid#15.

adenexter

Once I resolved the issue with applying the last patch the echo test worked on an Xperia XA2 which was one of the earlier problem devices.

Not a lot stands out in the code otherwise.

I guess we'll need to sort out a process for promoting and reviewing amber-camera as well. The standout thing to me is there might be some similar push back to the separate spec file for the droidmedia plugin. It might be better received if it's in a separate project, or combined into a single spec with conditions.

rpm/0071-sailfishos-webrtc-Implement-video-capture-module.-JB.patch

d-grigorev · 2021-08-10T07:25:23Z

I guess we'll need to sort out a process for promoting and reviewing amber-camera as well.

The lawers gave permission to contibute it to github.com/sailfishos, so what are next steps?. You can leave comments in https://github.com/d-grigorev/amber-camera/pull/1.

It might be better received if it's in a separate project, or combined into a single spec with conditions.

Isn't that convenient to have multiple specs? amber-camera headers should be accessible from pj:oss, amber-camera-droid-plugin should be built in a (common) adaptation project. The spec will be chosen depending on the OBS project name - looks clear for me. What are the disadvantages?

adenexter · 2021-08-10T07:36:28Z

I guess we'll need to sort out a process for promoting and reviewing amber-camera as well.

The layers gave permission to contibute it to github.com/sailfishos, so what are next steps?. You can leave comments in d-grigorev/amber-camera#1.

An admin will need to create the project under the sailfishos group. Could you create a task in bugzilla requesting it?

It might be better received if it's in a separate project, or combined into a single spec with conditions.

Isn't that convenient to have multiple specs?

Refer to the discussion in the droidmedia split bug. I argued for separate files there because droidmedia in a unique project, but based on that I would expect a similar objection again.

d-grigorev · 2021-08-10T16:29:08Z

Changes from the previous version:

The patch now applies cleanly
Fixed reported issues
Fixed hardware-backed VP8 encoding using gmp-droid

d-grigorev · 2021-08-12T15:01:31Z

Changes from the previous version:

Change the name of the video input library to gecko-camera, see https://github.com/d-grigorev/gecko-camera/pull/1

adenexter

I still want to verify this works on a Jolla C and I checked in on the promotion build of the droidmedia changes this depends on and there's a couple compilation failures related to age of the build environment.

But if I can see this working there, I think I'm ready to approve.

d-grigorev · 2021-08-17T07:39:10Z

I still want to verify this works on a Jolla C and I checked in on the promotion build of the droidmedia changes this depends on and there's a couple compilation failures related to age of the build environment.

Unfortunately I don't have a Jolla C. The code for YCbCr template may need to be fixed in gecko-camera, if the picture is weird.

But if I can see this working there, I think I'm ready to approve.

Thank you. I have a fix for audio getting out of sync with video in the incoming stream, but I think it is matter of the following commits.

adenexter · 2021-08-19T08:14:38Z

Unfortunately I don't have a Jolla C. The code for YCbCr template may need to be fixed in gecko-camera, if the picture is weird.

The droidmedia promotion has succeeded on that hardware now. And it appears to work without any display issues.

okodron

Looks like it's ready now.

d-grigorev marked this pull request as draft June 25, 2021 18:33

abranson reviewed Jun 28, 2021

View reviewed changes

d-grigorev force-pushed the jb53982 branch from 9f621ad to 5f33e2f Compare July 1, 2021 18:34

d-grigorev marked this pull request as ready for review July 1, 2021 18:34

llewelld reviewed Jul 2, 2021

View reviewed changes

adenexter reviewed Jul 5, 2021

View reviewed changes

rpm/0070-sailfishos-webrtc-Implement-video-capture-module.-JB.patch Outdated Show resolved Hide resolved

rpm/0070-sailfishos-webrtc-Implement-video-capture-module.-JB.patch Outdated Show resolved Hide resolved

d-grigorev marked this pull request as draft July 7, 2021 07:57

d-grigorev force-pushed the jb53982 branch 2 times, most recently from f0cdb8f to d31b1f3 Compare August 6, 2021 19:46

d-grigorev marked this pull request as ready for review August 6, 2021 19:53

d-grigorev requested review from adenexter, llewelld and abranson August 9, 2021 16:58

adenexter suggested changes Aug 10, 2021

View reviewed changes

d-grigorev force-pushed the jb53982 branch from d31b1f3 to 2e27f6d Compare August 10, 2021 16:17

d-grigorev requested a review from adenexter August 10, 2021 16:33

[sailfishos][webrtc] Enable support for WebRTC video. JB#53982

8667b39

d-grigorev force-pushed the jb53982 branch from 2e27f6d to 8667b39 Compare August 12, 2021 14:59

adenexter reviewed Aug 17, 2021

View reviewed changes

d-grigorev requested a review from adenexter August 18, 2021 10:45

adenexter approved these changes Aug 19, 2021

View reviewed changes

okodron approved these changes Aug 19, 2021

View reviewed changes

adenexter merged commit 6bf0c08 into sailfishos:master Aug 27, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[sailfishos][webrtc] Enable support for WebRTC video. JB#53982 #3

[sailfishos][webrtc] Enable support for WebRTC video. JB#53982 #3

d-grigorev commented Jun 25, 2021

abranson Jun 28, 2021

d-grigorev Jun 28, 2021 •

edited

llewelld left a comment •

edited

llewelld Jul 2, 2021

d-grigorev Jul 2, 2021

llewelld Jul 2, 2021

d-grigorev Jul 2, 2021

llewelld Jul 2, 2021

llewelld Jul 2, 2021

d-grigorev Jul 2, 2021

llewelld Jul 2, 2021

llewelld Jul 2, 2021

llewelld Jul 2, 2021

d-grigorev commented Jul 2, 2021

d-grigorev commented Aug 7, 2021 •

edited

d-grigorev commented Aug 7, 2021

d-grigorev commented Aug 9, 2021

adenexter left a comment

d-grigorev commented Aug 10, 2021 •

edited

adenexter commented Aug 10, 2021

d-grigorev commented Aug 10, 2021

d-grigorev commented Aug 12, 2021

adenexter left a comment

d-grigorev commented Aug 17, 2021

adenexter commented Aug 19, 2021

okodron left a comment

[sailfishos][webrtc] Enable support for WebRTC video. JB#53982 #3

[sailfishos][webrtc] Enable support for WebRTC video. JB#53982 #3

Conversation

d-grigorev commented Jun 25, 2021

Choose a reason for hiding this comment

d-grigorev Jun 28, 2021 • edited

Choose a reason for hiding this comment

llewelld left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

d-grigorev commented Jul 2, 2021

d-grigorev commented Aug 7, 2021 • edited

d-grigorev commented Aug 7, 2021

d-grigorev commented Aug 9, 2021

adenexter left a comment

Choose a reason for hiding this comment

d-grigorev commented Aug 10, 2021 • edited

adenexter commented Aug 10, 2021

d-grigorev commented Aug 10, 2021

d-grigorev commented Aug 12, 2021

adenexter left a comment

Choose a reason for hiding this comment

d-grigorev commented Aug 17, 2021

adenexter commented Aug 19, 2021

okodron left a comment

Choose a reason for hiding this comment

d-grigorev Jun 28, 2021 •

edited

llewelld left a comment •

edited

d-grigorev commented Aug 7, 2021 •

edited

d-grigorev commented Aug 10, 2021 •

edited