1223312 - webrtc Module Build error on MIPS Platform and web page crashed during webrtc . such as Loongson3A

Status: RESOLVED FIXED

(Core :: WebRTC, defect)

Description

[huangwenjun06]

Attachment: 0001-1-SRTP-fails-to-initialize-on-mips.-switch-to-use-CP.patch

User Agent: Mozilla/5.0 (Windows NT 6.1) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/46.0.2490.80 Safari/537.36

Steps to reproduce:

Test firefox webrtc mechanism
1:start Firefox on mips Platform. such as Loongson3A
2: visit URL: https://mozilla.github.io/webrtc-landing/pc_test.html 
  


Actual results:

Test failed. Could Transfer video.


Expected results:

Could RealTime Communication

Comment 1

[huangwenjun06]

Attachment: result.png

Comment 2

[Randell Jesup [:jesup] (needinfo me)]

We don't handle MIPS as a primary platform; however patches are accepted and will be reviewed.

Comment 3

[huangwenjun06]

Attachment: 0001-Patch1-default-enable-webrtc-on-MIPS64-platform-such.patch

Comment 4

[huangwenjun06]

Attachment: 0002-Patch2-SRTP-fails-to-initialize-on-mips-switch-to-us.patch

Comment 5

[huangwenjun06]

Attachment: 0002-Patch2-Disable-Noise-reduction-same-as-ARM.-Solve-we.patch

Comment 6

[huangwenjun06]

Attachment: webrtc_gecko_result.jpg

Comment 7

[huangwenjun06]

Hi, Randell Jesup. 
  When i download latest gecko-dev code. checkout to inbound branch. I found webrtc mechanism still could not run on MIPS64 platform, such as loongson3a. So i resubmit three patches. I hope you review them。

Comment 8

[Nils Ohlmeier [:drno]]

Comment on attachment 8926275 [details] [diff] [review]
0001-Patch1-default-enable-webrtc-on-MIPS64-platform-such.patch

Review of attachment 8926275 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM

Comment 9

[Nils Ohlmeier [:drno]]

Comment on attachment 8926276 [details] [diff] [review]
0002-Patch2-SRTP-fails-to-initialize-on-mips-switch-to-us.patch

Review of attachment 8926276 [details] [diff] [review]:
-----------------------------------------------------------------

This doesn't look right to me.

::: netwerk/srtp/src/moz.build
@@ +56,1 @@
>      DEFINES['CPU_CISC'] = 1

Isn't MIPS suppose to be a RISC architecture?

Does this mean something in libsrtp is broken?

Comment 10

[Nils Ohlmeier [:drno]]

Comment on attachment 8926277 [details] [diff] [review]
0002-Patch2-Disable-Noise-reduction-same-as-ARM.-Solve-we.patch

Review of attachment 8926277 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM

Comment 11

[huangwenjun06]

arm should also be RISC architecture, libsrtp also set CPU_CISC.
testCase: dom/media/tests/mochitest/test_peerConnection_verifyAudioAfterRenegotiation.html
(In reply to Nils Ohlmeier [:drno] from comment #9)
> Comment on attachment 8926276 [details] [diff] [review]
> 0002-Patch2-SRTP-fails-to-initialize-on-mips-switch-to-us.patch
> 
> Review of attachment 8926276 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This doesn't look right to me.
> 
> ::: netwerk/srtp/src/moz.build
> @@ +56,1 @@
> >      DEFINES['CPU_CISC'] = 1
> 
> Isn't MIPS suppose to be a RISC architecture?
> 
> Does this mean something in libsrtp is broken?

(In reply to Nils Ohlmeier [:drno] from comment #9)
> Comment on attachment 8926276 [details] [diff] [review]
> 0002-Patch2-SRTP-fails-to-initialize-on-mips-switch-to-us.patch
> 
> Review of attachment 8926276 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This doesn't look right to me.
> 
> ::: netwerk/srtp/src/moz.build
> @@ +56,1 @@
> >      DEFINES['CPU_CISC'] = 1
> 
> Isn't MIPS suppose to be a RISC architecture?
> 
> Does this mean something in libsrtp is broken?

Comment 12

[huangwenjun06]

Comment on attachment 8926277 [details] [diff] [review]
0002-Patch2-Disable-Noise-reduction-same-as-ARM.-Solve-we.patch

>From 77aca67fbde44f87acaec298b471fead2175c573 Mon Sep 17 00:00:00 2001
>From: huangwenjun <[e-mail]>
>Date: Wed, 8 Nov 2017 14:53:23 +0800
>Subject: [PATCH 3/3] Patch3: Disable Noise reduction same as ARM. Solve web
> page crash during webrtc.
>
>---
> .../webrtc/trunk/webrtc/modules/video_coding/codecs/vp8/vp8_impl.cc | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
>diff --git a/media/webrtc/trunk/webrtc/modules/video_coding/codecs/vp8/vp8_impl.cc b/media/webrtc/trunk/webrtc/modules/video_coding/codecs/vp8/vp8_impl.cc
>index 0ecb984..00dd70e 100644
>--- a/media/webrtc/trunk/webrtc/modules/video_coding/codecs/vp8/vp8_impl.cc
>+++ b/media/webrtc/trunk/webrtc/modules/video_coding/codecs/vp8/vp8_impl.cc
>@@ -607,7 +607,7 @@ int VP8EncoderImpl::InitAndSetControlSettings() {
>   // when encoding lower resolution streams. Would it work with the
>   // multi-res encoding feature?
>   denoiserState denoiser_state = kDenoiserOnYOnly;
>-#if defined(WEBRTC_ARCH_ARM) || defined(WEBRTC_ARCH_ARM64) || defined(ANDROID)
>+#if defined(WEBRTC_ARCH_ARM) || defined(WEBRTC_ARCH_ARM64) || defined(ANDROID) || defined(WEBRTC_ARCH_MIPS)
>   denoiser_state = kDenoiserOnYOnly;
> #else
>   denoiser_state = kDenoiserOnAdaptive;
>@@ -1033,7 +1033,7 @@ int VP8DecoderImpl::InitDecode(const VideoCodec* inst, int number_of_cores) {
> 
>   vpx_codec_flags_t flags = 0;
> #if !defined(WEBRTC_ARCH_ARM) && !defined(WEBRTC_ARCH_ARM64) && \
>-  !defined(ANDROID)
>+  !defined(ANDROID) && !defined(WEBRTC_ARCH_MIPS)
>   flags = VPX_CODEC_USE_POSTPROC;
> #endif
> 
>@@ -1074,7 +1074,7 @@ int VP8DecoderImpl::Decode(const EncodedImage& input_image,
>   }
> 
> #if !defined(WEBRTC_ARCH_ARM) && !defined(WEBRTC_ARCH_ARM64) && \
>-  !defined(ANDROID)
>+  !defined(ANDROID) && !defined(WEBRTC_ARCH_MIPS)
>   vp8_postproc_cfg_t ppcfg;
>   // MFQE enabled to reduce key frame popping.
>   ppcfg.post_proc_flag = VP8_MFQE | VP8_DEBLOCK;
>-- 
>2.1.0
>

Comment 13

[huangwenjun06]

Attachment: Delete Patch 0002-Patch2-SRTP-fails-to-initialize-on-mips-switch-to-us.patch.

Delete Patch 0002-Patch2-SRTP-fails-to-initialize-on-mips-switch-to-us.patch.
I would open a bug to fix the problem.

Comment 14

[huangwenjun06]

Comment on attachment 8926277 [details] [diff] [review]
0002-Patch2-Disable-Noise-reduction-same-as-ARM.-Solve-we.patch

>From 77aca67fbde44f87acaec298b471fead2175c573 Mon Sep 17 00:00:00 2001
>From: huangwenjun <[e-mail]>
>Date: Wed, 8 Nov 2017 14:53:23 +0800
>Subject: [PATCH 2/2] Patch2: Disable Noise reduction same as ARM. Solve web
> page crash during webrtc.
>
>---
> .../webrtc/trunk/webrtc/modules/video_coding/codecs/vp8/vp8_impl.cc | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
>diff --git a/media/webrtc/trunk/webrtc/modules/video_coding/codecs/vp8/vp8_impl.cc b/media/webrtc/trunk/webrtc/modules/video_coding/codecs/vp8/vp8_impl.cc
>index 0ecb984..00dd70e 100644
>--- a/media/webrtc/trunk/webrtc/modules/video_coding/codecs/vp8/vp8_impl.cc
>+++ b/media/webrtc/trunk/webrtc/modules/video_coding/codecs/vp8/vp8_impl.cc
>@@ -607,7 +607,7 @@ int VP8EncoderImpl::InitAndSetControlSettings() {
>   // when encoding lower resolution streams. Would it work with the
>   // multi-res encoding feature?
>   denoiserState denoiser_state = kDenoiserOnYOnly;
>-#if defined(WEBRTC_ARCH_ARM) || defined(WEBRTC_ARCH_ARM64) || defined(ANDROID)
>+#if defined(WEBRTC_ARCH_ARM) || defined(WEBRTC_ARCH_ARM64) || defined(ANDROID) || defined(WEBRTC_ARCH_MIPS)
>   denoiser_state = kDenoiserOnYOnly;
> #else
>   denoiser_state = kDenoiserOnAdaptive;
>@@ -1033,7 +1033,7 @@ int VP8DecoderImpl::InitDecode(const VideoCodec* inst, int number_of_cores) {
> 
>   vpx_codec_flags_t flags = 0;
> #if !defined(WEBRTC_ARCH_ARM) && !defined(WEBRTC_ARCH_ARM64) && \
>-  !defined(ANDROID)
>+  !defined(ANDROID) && !defined(WEBRTC_ARCH_MIPS)
>   flags = VPX_CODEC_USE_POSTPROC;
> #endif
> 
>@@ -1074,7 +1074,7 @@ int VP8DecoderImpl::Decode(const EncodedImage& input_image,
>   }
> 
> #if !defined(WEBRTC_ARCH_ARM) && !defined(WEBRTC_ARCH_ARM64) && \
>-  !defined(ANDROID)
>+  !defined(ANDROID) && !defined(WEBRTC_ARCH_MIPS)
>   vp8_postproc_cfg_t ppcfg;
>   // MFQE enabled to reduce key frame popping.
>   ppcfg.post_proc_flag = VP8_MFQE | VP8_DEBLOCK;
>-- 
>2.1.0
>

Comment 15

[huangwenjun06]

Comment on attachment 8926277 [details] [diff] [review]
0002-Patch2-Disable-Noise-reduction-same-as-ARM.-Solve-we.patch

>From 77aca67fbde44f87acaec298b471fead2175c573 Mon Sep 17 00:00:00 2001
>From: huangwenjun <[e-mail]>
>Date: Wed, 8 Nov 2017 14:53:23 +0800
>Subject: [PATCH 2/2] Patch2: Disable Noise reduction same as ARM. Solve web
> page crash during webrtc.
>
>---
> .../webrtc/trunk/webrtc/modules/video_coding/codecs/vp8/vp8_impl.cc | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
>diff --git a/media/webrtc/trunk/webrtc/modules/video_coding/codecs/vp8/vp8_impl.cc b/media/webrtc/trunk/webrtc/modules/video_coding/codecs/vp8/vp8_impl.cc
>index 0ecb984..00dd70e 100644
>--- a/media/webrtc/trunk/webrtc/modules/video_coding/codecs/vp8/vp8_impl.cc
>+++ b/media/webrtc/trunk/webrtc/modules/video_coding/codecs/vp8/vp8_impl.cc
>@@ -607,7 +607,7 @@ int VP8EncoderImpl::InitAndSetControlSettings() {
>   // when encoding lower resolution streams. Would it work with the
>   // multi-res encoding feature?
>   denoiserState denoiser_state = kDenoiserOnYOnly;
>-#if defined(WEBRTC_ARCH_ARM) || defined(WEBRTC_ARCH_ARM64) || defined(ANDROID)
>+#if defined(WEBRTC_ARCH_ARM) || defined(WEBRTC_ARCH_ARM64) || defined(ANDROID) || defined(WEBRTC_ARCH_MIPS)
>   denoiser_state = kDenoiserOnYOnly;
> #else
>   denoiser_state = kDenoiserOnAdaptive;
>@@ -1033,7 +1033,7 @@ int VP8DecoderImpl::InitDecode(const VideoCodec* inst, int number_of_cores) {
> 
>   vpx_codec_flags_t flags = 0;
> #if !defined(WEBRTC_ARCH_ARM) && !defined(WEBRTC_ARCH_ARM64) && \
>-  !defined(ANDROID)
>+  !defined(ANDROID) && !defined(WEBRTC_ARCH_MIPS)
>   flags = VPX_CODEC_USE_POSTPROC;
> #endif
> 
>@@ -1074,7 +1074,7 @@ int VP8DecoderImpl::Decode(const EncodedImage& input_image,
>   }
> 
> #if !defined(WEBRTC_ARCH_ARM) && !defined(WEBRTC_ARCH_ARM64) && \
>-  !defined(ANDROID)
>+  !defined(ANDROID) && !defined(WEBRTC_ARCH_MIPS)
>   vp8_postproc_cfg_t ppcfg;
>   // MFQE enabled to reduce key frame popping.
>   ppcfg.post_proc_flag = VP8_MFQE | VP8_DEBLOCK;
>-- 
>2.1.0
>

Comment 16

[huangwenjun06]

Attachment: 0001-Patch1-default-enable-webrtc-on-MIPS64-platform-such.patch

Replace 0001-Patch1-default-enable-webrtc-on-MIPS64-platform-such.patch

Comment 17

[huangwenjun06]

Attachment: 0002-Patch2-Disable-Noise-reduction-same-as-ARM.-Solve-we.patch

Replace 0002-Patch2-Disable-Noise-reduction-same-as-ARM.-Solve-we.patch

Comment 18

[huangwenjun06]

Attachment: 0002-Patch2-Disable-Noise-reduction-same-as-ARM.-Solve-we.patch

Replace 0002-Patch2-Disable-Noise-reduction-same-as-ARM.-Solve-we.patch

Comment 19

[huangwenjun06]

Attachment: 0002-Patch2-Disable-Noise-reduction-same-as-ARM.-Solve-we.patch

Comment 20

[huangwenjun06]

hi,Nils Ohlmeier
I reorder these three patch. Delete 0002-Patch2-SRTP-fails-to-initialize-on-mips-switch-to-us.patch.
I hope you can review it.

Comment 21

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8926670 [details] [diff] [review]
0001-Patch1-default-enable-webrtc-on-MIPS64-platform-such.patch

Review of attachment 8926670 [details] [diff] [review]:
-----------------------------------------------------------------

r+ for the build_config.h change.  Adding r? for a build peer for moz.configure.

Comment 22

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8926675 [details] [diff] [review]
0002-Patch2-Disable-Noise-reduction-same-as-ARM.-Solve-we.patch

Review of attachment 8926675 [details] [diff] [review]:
-----------------------------------------------------------------

stealing review

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7184839aee72
Part 1: Enable webrtc on MIPS64 platforms by default. r=drno, r=jesup, r=ted
https://hg.mozilla.org/integration/mozilla-inbound/rev/f3a200c2049f
Part 2: Disable Noise reduction same as ARM. r=jesup

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7184839aee72
https://hg.mozilla.org/mozilla-central/rev/f3a200c2049f