Closed Bug 1223312 Opened 4 years ago Closed 2 years ago

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

Categories

(Core :: WebRTC, defect)

Other
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed
Blocking Flags:
backlog parking-lot

People

(Reporter: hwjeastd07, Assigned: hwjeastd07)

Details

Attachments

(3 files, 8 obsolete files)

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
Component: Untriaged → WebRTC
Product: Firefox → Core
backlog: --- → parking-lot
Attached image result.png (obsolete) —
We don't handle MIPS as a primary platform; however patches are accepted and will be reviewed.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(hwjeastd07)
Flags: needinfo?(hwjeastd07)
OS: Unspecified → Linux
Hardware: Unspecified → Other
Version: 41 Branch → Trunk
Attachment #8685349 - Attachment is obsolete: true
Flags: needinfo?(drno)
Attached image webrtc_gecko_result.jpg
Attachment #8685720 - Attachment is obsolete: true
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 on attachment 8926275 [details] [diff] [review]
0001-Patch1-default-enable-webrtc-on-MIPS64-platform-such.patch

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

LGTM
Attachment #8926275 - Flags: review+
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 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
Attachment #8926277 - Flags: review+
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 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 <huangwenjun-hf@loongson.cn>
>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
>
Attachment #8926277 - Attachment description: 0003-Patch3-Disable-Noise-reduction-same-as-ARM.-Solve-we.patch → 0002-Patch2-Disable-Noise-reduction-same-as-ARM.-Solve-we.patch
Delete Patch 0002-Patch2-SRTP-fails-to-initialize-on-mips-switch-to-us.patch.
I would open a bug to fix the problem.
Attachment #8926276 - Attachment is obsolete: true
Keywords: checkin-needed
Summary: webrtc could not use on MIPS Platform . such as Loongson3A → webrtc Module Build error on MIPS Platform and web page crashed during webrtc . such as Loongson3A
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 <huangwenjun-hf@loongson.cn>
>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 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 <huangwenjun-hf@loongson.cn>
>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
>
Attachment #8926277 - Attachment filename: 0003-Patch3-Disable-Noise-reduction-same-as-ARM.-Solve-we.patch → 0002-Patch2-Disable-Noise-reduction-same-as-ARM.-Solve-we.patch
Replace 0001-Patch1-default-enable-webrtc-on-MIPS64-platform-such.patch
Attachment #8926275 - Attachment is obsolete: true
Replace 0002-Patch2-Disable-Noise-reduction-same-as-ARM.-Solve-we.patch
Attachment #8926277 - Attachment is obsolete: true
Replace 0002-Patch2-Disable-Noise-reduction-same-as-ARM.-Solve-we.patch
Attachment #8926664 - Attachment is obsolete: true
Attachment #8926671 - Attachment is obsolete: true
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.
Keywords: checkin-needed
Attachment #8926670 - Flags: review?(rjesup)
Attachment #8926675 - Flags: review?(drno)
Attachment #8926670 - Flags: review?(rjesup)
Attachment #8926670 - Flags: review?(drno)
Attachment #8926670 - Flags: review?(drno) → review+
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.
Attachment #8926670 - Flags: review?(drno)
Attachment #8926670 - Flags: review?(core-build-config-reviews)
Attachment #8926670 - Flags: review+
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
Attachment #8926675 - Flags: review?(drno) → review+
Attachment #8926670 - Flags: review?(core-build-config-reviews) → review+
Assignee: nobody → hwjeastd07
Keywords: checkin-needed
Flags: needinfo?(drno)
Pushed by ryanvm@gmail.com:
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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/7184839aee72
https://hg.mozilla.org/mozilla-central/rev/f3a200c2049f
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.