Closed Bug 1029983 Opened 5 years ago Closed 5 years ago

H.264 codec is working on B2G ignoring preference 'media.peerconnection.video.h264_enabled'

Categories

(Core :: WebRTC, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
2.0 S5 (4july)
blocking-b2g 2.0+
Tracking Status
firefox31 --- wontfix
firefox32 --- fixed
firefox33 --- fixed
b2g-v2.0 --- fixed
b2g-v2.1 --- fixed

People

(Reporter: pchangbin, Assigned: pchangbin)

Details

Attachments

(1 file, 2 obsolete files)

H.264 codec is working no matter what the preference 'media.peerconnection.video.h264_enabled' is set to false or not.
This occurs on KitKat based B2G device.
Return VCM_CODEC_RESOURCE_H264 from VcmSIPCCBinding::getVideoCodecsHw() makes H.264 working.
This makes the preference is checked before using OMX codec.
Attachment #8445723 - Flags: review?(ethanhugg)
Attachment #8445723 - Attachment is patch: true
Comment on attachment 8445723 [details] [diff] [review]
Perform reserve omx codec only when preference "media.peerconnection.video.h264_enabled" is set to true.

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

Looks good to me.  Thanks.
Attachment #8445723 - Flags: review?(ethanhugg) → review+
Keywords: checkin-needed
Assignee: nobody → pchangbin
Can we please run this through Try to at least confirm that it builds?
Keywords: checkin-needed
Also, this doesn't apply cleanly to mozilla-inbound anyway. Please rebase. Also, make sure your patch is generated per the link below (to include appropriate levels of context and the like).
https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #3)
> Can we please run this through Try to at least confirm that it builds?
Did you experience any problem with this patch?
I checked it with my own B2G build and it was fine.
It's not easy for me to find a point that makes an error.
I think, it includes proper header and is enclosed with preprocessor directives.

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #4)
> Also, this doesn't apply cleanly to mozilla-inbound anyway. Please rebase.
I'm not using Mercurial but git. So, I checked in-bound branch to check if there's any probability for conflict.(http://git.mozilla.org/?p=integration/gecko-dev.git;a=blob;f=media/webrtc/signaling/src/media/VcmSIPCCBinding.cpp;hb=HEAD)
There's no changed since 14th, June. I'm seeing, obviously, it's identical with my base source of the patch.
> Also, make sure your patch is generated per the link below (to include
> appropriate levels of context and the like).
> https://developer.mozilla.org/en-US/docs/
> Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-
> in_for_me.3F
I'll change my patch to "Mercurial-ready" as you guided, after got answers about above.
Flags: needinfo?(ryanvm)
(In reply to Changbin Park from comment #5)
> (In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #3)
> > Can we please run this through Try to at least confirm that it builds?
> Did you experience any problem with this patch?
> I checked it with my own B2G build and it was fine.
> It's not easy for me to find a point that makes an error.
> I think, it includes proper header and is enclosed with preprocessor
> directives.

Per the policy update sent out 6 weeks ago, it's a requirement:
https://groups.google.com/d/msg/mozilla.dev.platform/9w0-Bh_3vVI/xWebYK64GdwJ

> (In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #4)
> > Also, this doesn't apply cleanly to mozilla-inbound anyway. Please rebase.
> I'm not using Mercurial but git. So, I checked in-bound branch to check if
> there's any probability for
> conflict.(http://git.mozilla.org/?p=integration/gecko-dev.git;a=blob;f=media/
> webrtc/signaling/src/media/VcmSIPCCBinding.cpp;hb=HEAD)
> There's no changed since 14th, June. I'm seeing, obviously, it's identical
> with my base source of the patch.

patching file media/webrtc/signaling/src/media/VcmSIPCCBinding.cpp
Hunk #1 FAILED at 33
Hunk #2 FAILED at 256
2 out of 2 hunks FAILED -- saving rejects to file media/webrtc/signaling/src/media/VcmSIPCCBinding.cpp.rej

> > Also, make sure your patch is generated per the link below (to include
> > appropriate levels of context and the like).
> > https://developer.mozilla.org/en-US/docs/
> > Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-
> > in_for_me.3F
> I'll change my patch to "Mercurial-ready" as you guided, after got answers
> about above.

Thank you.
Flags: needinfo?(ryanvm)
I'm attaching hg-patch instead of git-patch.
It's converted by git-patch-to-hg-patch in moz-git-tools
Attachment #8445723 - Attachment is obsolete: true
Comment on attachment 8448401 [details] [diff] [review]
[hg-patch] Perform reserve omx codec only when preference "media.peerconnection.video.h264_enabled" is set to true.

># HG changeset patch
># User Changbin Park <pchangbin@gmail.com>
>
>Bug 1029983 - H.264 codec is working on B2G ignoring preference 'media.peerconnection.video.h264_enabled'
>
>Change-Id: I8b853a023cc6fd8732d37b85c41b0cf9dfe3fb7e
>---
> .../webrtc/signaling/src/media/VcmSIPCCBinding.cpp |   25 +++++++++++++-------
> 1 file changed, 16 insertions(+), 9 deletions(-)
>
>diff --git a/media/webrtc/signaling/src/media/VcmSIPCCBinding.cpp b/media/webrtc/signaling/src/media/VcmSIPCCBinding.cpp
>index ad32c1e..3810584 100644
>--- a/media/webrtc/signaling/src/media/VcmSIPCCBinding.cpp
>+++ b/media/webrtc/signaling/src/media/VcmSIPCCBinding.cpp
>@@ -34,6 +34,7 @@
> #ifdef MOZILLA_INTERNAL_API
> #include "nsIPrincipal.h"
> #include "nsIDocument.h"
>+#include "mozilla/Preferences.h"
> #endif
> 
> #include <stdlib.h>
>@@ -257,17 +258,23 @@ int VcmSIPCCBinding::getVideoCodecsHw()
>   // Note that currently, OMXCodecReservation needs to be held by an sp<> because it puts
>   // 'this' into an sp<EventListener> to talk to the resource reservation code
> #ifdef MOZ_WEBRTC_OMX
>-  android::sp<android::OMXCodecReservation> encode = new android::OMXCodecReservation(true);
>-  android::sp<android::OMXCodecReservation> decode = new android::OMXCodecReservation(false);
>-
>-  // Currently we just check if they're available right now, which will fail if we're
>-  // trying to call ourself, for example.  It will work for most real-world cases, like
>-  // if we try to add a person to a 2-way call to make a 3-way mesh call
>-  if (encode->ReserveOMXCodec() && decode->ReserveOMXCodec()) {
>-    CSFLogDebug( logTag, "%s: H264 hardware codec available", __FUNCTION__);
>-    return VCM_CODEC_RESOURCE_H264;
>+#ifdef MOZILLA_INTERNAL_API
>+  if (Preferences::GetBool("media.peerconnection.video.h264_enabled")) {
>+#endif
>+    android::sp<android::OMXCodecReservation> encode = new android::OMXCodecReservation(true);
>+    android::sp<android::OMXCodecReservation> decode = new android::OMXCodecReservation(false);
>+
>+    // Currently we just check if they're available right now, which will fail if we're
>+    // trying to call ourself, for example.  It will work for most real-world cases, like
>+    // if we try to add a person to a 2-way call to make a 3-way mesh call
>+    if (encode->ReserveOMXCodec() && decode->ReserveOMXCodec()) {
>+      CSFLogDebug( logTag, "%s: H264 hardware codec available", __FUNCTION__);
>+      return VCM_CODEC_RESOURCE_H264;
>+    }
>+#if defined( MOZILLA_INTERNAL_API)
>   }
> #endif
>+#endif
> 
>   return 0;
> }
>-- 
>1.7.9.5
>
Attachment #8448401 - Attachment is patch: true
Comment on attachment 8448401 [details] [diff] [review]
[hg-patch] Perform reserve omx codec only when preference "media.peerconnection.video.h264_enabled" is set to true.

># HG changeset patch
># User Changbin Park <pchangbin@gmail.com>
>
>Bug 1029983 - H.264 codec is working on B2G ignoring preference 'media.peerconnection.video.h264_enabled'
>
>Change-Id: I8b853a023cc6fd8732d37b85c41b0cf9dfe3fb7e
>---
> .../webrtc/signaling/src/media/VcmSIPCCBinding.cpp |   25 +++++++++++++-------
> 1 file changed, 16 insertions(+), 9 deletions(-)
>
>diff --git a/media/webrtc/signaling/src/media/VcmSIPCCBinding.cpp b/media/webrtc/signaling/src/media/VcmSIPCCBinding.cpp
>index ad32c1e..3810584 100644
>--- a/media/webrtc/signaling/src/media/VcmSIPCCBinding.cpp
>+++ b/media/webrtc/signaling/src/media/VcmSIPCCBinding.cpp
>@@ -34,6 +34,7 @@
> #ifdef MOZILLA_INTERNAL_API
> #include "nsIPrincipal.h"
> #include "nsIDocument.h"
>+#include "mozilla/Preferences.h"
> #endif
> 
> #include <stdlib.h>
>@@ -257,17 +258,23 @@ int VcmSIPCCBinding::getVideoCodecsHw()
>   // Note that currently, OMXCodecReservation needs to be held by an sp<> because it puts
>   // 'this' into an sp<EventListener> to talk to the resource reservation code
> #ifdef MOZ_WEBRTC_OMX
>-  android::sp<android::OMXCodecReservation> encode = new android::OMXCodecReservation(true);
>-  android::sp<android::OMXCodecReservation> decode = new android::OMXCodecReservation(false);
>-
>-  // Currently we just check if they're available right now, which will fail if we're
>-  // trying to call ourself, for example.  It will work for most real-world cases, like
>-  // if we try to add a person to a 2-way call to make a 3-way mesh call
>-  if (encode->ReserveOMXCodec() && decode->ReserveOMXCodec()) {
>-    CSFLogDebug( logTag, "%s: H264 hardware codec available", __FUNCTION__);
>-    return VCM_CODEC_RESOURCE_H264;
>+#ifdef MOZILLA_INTERNAL_API
>+  if (Preferences::GetBool("media.peerconnection.video.h264_enabled")) {
>+#endif
>+    android::sp<android::OMXCodecReservation> encode = new android::OMXCodecReservation(true);
>+    android::sp<android::OMXCodecReservation> decode = new android::OMXCodecReservation(false);
>+
>+    // Currently we just check if they're available right now, which will fail if we're
>+    // trying to call ourself, for example.  It will work for most real-world cases, like
>+    // if we try to add a person to a 2-way call to make a 3-way mesh call
>+    if (encode->ReserveOMXCodec() && decode->ReserveOMXCodec()) {
>+      CSFLogDebug( logTag, "%s: H264 hardware codec available", __FUNCTION__);
>+      return VCM_CODEC_RESOURCE_H264;
>+    }
>+#if defined( MOZILLA_INTERNAL_API)
>   }
> #endif
>+#endif
> 
>   return 0;
> }
>-- 
>1.7.9.5
>
Attachment #8448401 - Attachment is obsolete: true
Comment on attachment 8445723 [details] [diff] [review]
Perform reserve omx codec only when preference "media.peerconnection.video.h264_enabled" is set to true.

# HG changeset patch
# User Changbin Park <pchangbin@gmail.com>

Bug 1029983 - H.264 codec is working on B2G ignoring preference 'media.peerconnection.video.h264_enabled'

Change-Id: I8b853a023cc6fd8732d37b85c41b0cf9dfe3fb7e
---
 .../webrtc/signaling/src/media/VcmSIPCCBinding.cpp |   25 +++++++++++++-------
 1 file changed, 16 insertions(+), 9 deletions(-)

diff --git a/media/webrtc/signaling/src/media/VcmSIPCCBinding.cpp b/media/webrtc/signaling/src/media/VcmSIPCCBinding.cpp
index ad32c1e..3810584 100644
--- a/media/webrtc/signaling/src/media/VcmSIPCCBinding.cpp
+++ b/media/webrtc/signaling/src/media/VcmSIPCCBinding.cpp
@@ -34,6 +34,7 @@
 #ifdef MOZILLA_INTERNAL_API
 #include "nsIPrincipal.h"
 #include "nsIDocument.h"
+#include "mozilla/Preferences.h"
 #endif
 
 #include <stdlib.h>
@@ -257,17 +258,23 @@ int VcmSIPCCBinding::getVideoCodecsHw()
   // Note that currently, OMXCodecReservation needs to be held by an sp<> because it puts
   // 'this' into an sp<EventListener> to talk to the resource reservation code
 #ifdef MOZ_WEBRTC_OMX
-  android::sp<android::OMXCodecReservation> encode = new android::OMXCodecReservation(true);
-  android::sp<android::OMXCodecReservation> decode = new android::OMXCodecReservation(false);
-
-  // Currently we just check if they're available right now, which will fail if we're
-  // trying to call ourself, for example.  It will work for most real-world cases, like
-  // if we try to add a person to a 2-way call to make a 3-way mesh call
-  if (encode->ReserveOMXCodec() && decode->ReserveOMXCodec()) {
-    CSFLogDebug( logTag, "%s: H264 hardware codec available", __FUNCTION__);
-    return VCM_CODEC_RESOURCE_H264;
+#ifdef MOZILLA_INTERNAL_API
+  if (Preferences::GetBool("media.peerconnection.video.h264_enabled")) {
+#endif
+    android::sp<android::OMXCodecReservation> encode = new android::OMXCodecReservation(true);
+    android::sp<android::OMXCodecReservation> decode = new android::OMXCodecReservation(false);
+
+    // Currently we just check if they're available right now, which will fail if we're
+    // trying to call ourself, for example.  It will work for most real-world cases, like
+    // if we try to add a person to a 2-way call to make a 3-way mesh call
+    if (encode->ReserveOMXCodec() && decode->ReserveOMXCodec()) {
+      CSFLogDebug( logTag, "%s: H264 hardware codec available", __FUNCTION__);
+      return VCM_CODEC_RESOURCE_H264;
+    }
+#if defined( MOZILLA_INTERNAL_API)
   }
 #endif
+#endif
 
   return 0;
 }
-- 
1.7.9.5
Attachment #8445723 - Attachment is obsolete: false
Comment on attachment 8445723 [details] [diff] [review]
Perform reserve omx codec only when preference "media.peerconnection.video.h264_enabled" is set to true.

# HG changeset patch
# User Changbin Park <pchangbin@gmail.com>

Bug 1029983 - H.264 codec is working on B2G ignoring preference 'media.peerconnection.video.h264_enabled' r=ethanhugg

Change-Id: I8b853a023cc6fd8732d37b85c41b0cf9dfe3fb7e
---
 .../webrtc/signaling/src/media/VcmSIPCCBinding.cpp |   25 +++++++++++++-------
 1 file changed, 16 insertions(+), 9 deletions(-)

diff --git a/media/webrtc/signaling/src/media/VcmSIPCCBinding.cpp b/media/webrtc/signaling/src/media/VcmSIPCCBinding.cpp
index ad32c1e..3810584 100644
--- a/media/webrtc/signaling/src/media/VcmSIPCCBinding.cpp
+++ b/media/webrtc/signaling/src/media/VcmSIPCCBinding.cpp
@@ -34,6 +34,7 @@
 #ifdef MOZILLA_INTERNAL_API
 #include "nsIPrincipal.h"
 #include "nsIDocument.h"
+#include "mozilla/Preferences.h"
 #endif
 
 #include <stdlib.h>
@@ -257,17 +258,23 @@ int VcmSIPCCBinding::getVideoCodecsHw()
   // Note that currently, OMXCodecReservation needs to be held by an sp<> because it puts
   // 'this' into an sp<EventListener> to talk to the resource reservation code
 #ifdef MOZ_WEBRTC_OMX
-  android::sp<android::OMXCodecReservation> encode = new android::OMXCodecReservation(true);
-  android::sp<android::OMXCodecReservation> decode = new android::OMXCodecReservation(false);
-
-  // Currently we just check if they're available right now, which will fail if we're
-  // trying to call ourself, for example.  It will work for most real-world cases, like
-  // if we try to add a person to a 2-way call to make a 3-way mesh call
-  if (encode->ReserveOMXCodec() && decode->ReserveOMXCodec()) {
-    CSFLogDebug( logTag, "%s: H264 hardware codec available", __FUNCTION__);
-    return VCM_CODEC_RESOURCE_H264;
+#ifdef MOZILLA_INTERNAL_API
+  if (Preferences::GetBool("media.peerconnection.video.h264_enabled")) {
+#endif
+    android::sp<android::OMXCodecReservation> encode = new android::OMXCodecReservation(true);
+    android::sp<android::OMXCodecReservation> decode = new android::OMXCodecReservation(false);
+
+    // Currently we just check if they're available right now, which will fail if we're
+    // trying to call ourself, for example.  It will work for most real-world cases, like
+    // if we try to add a person to a 2-way call to make a 3-way mesh call
+    if (encode->ReserveOMXCodec() && decode->ReserveOMXCodec()) {
+      CSFLogDebug( logTag, "%s: H264 hardware codec available", __FUNCTION__);
+      return VCM_CODEC_RESOURCE_H264;
+    }
+#if defined( MOZILLA_INTERNAL_API)
   }
 #endif
+#endif
 
   return 0;
 }
-- 
1.7.9.5
Attachment #8445723 - Attachment is obsolete: true
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #6)
> Per the policy update sent out 6 weeks ago, it's a requirement:
> https://groups.google.com/d/msg/mozilla.dev.platform/9w0-Bh_3vVI/xWebYK64GdwJ
I checked the policy and realize that level 1 access is required.
So, I filed a bug 1032600 for the access.
Would you please vouch for me?
> patching file media/webrtc/signaling/src/media/VcmSIPCCBinding.cpp
> Hunk #1 FAILED at 33
> Hunk #2 FAILED at 256
> 2 out of 2 hunks FAILED -- saving rejects to file
> media/webrtc/signaling/src/media/VcmSIPCCBinding.cpp.rej
I'm still not understanding why the errors are occurred.
I'll recheck this with my own hg clone, after I got the access.
Thanks in advance.
Flags: needinfo?(ryanvm)
Done :)
Flags: needinfo?(ryanvm)
I could run a try build for you today, except you seem to be putting the patches into the comments instead of as an attachment.  If you are using hg now, bzexport would do this nicely for you.
Attachment #8448401 - Attachment is obsolete: false
Change-Id: I8b853a023cc6fd8732d37b85c41b0cf9dfe3fb7e
---
 .../webrtc/signaling/src/media/VcmSIPCCBinding.cpp |   25 +++++++++++++-------
 1 file changed, 16 insertions(+), 9 deletions(-)
Attachment #8448401 - Attachment is obsolete: true
Assignee: pchangbin → ethanhugg
Status: NEW → ASSIGNED
Your patch did not apply cleanly on M-C so I rebased it.  The line numbers of your VcmSIPCCBinding.cpp patch are different than mine.  I pushed this one to Try here:

https://tbpl.mozilla.org/?tree=Try&rev=cd0e82f1f7d0
Assignee: ethanhugg → pchangbin
Thank you very much Ethan. You're very thoughtful...
I'm still getting level 1 access account.
It seems no problem with build.
https://tbpl.mozilla.org/?tree=Try&rev=cd0e82f1f7d0
Keywords: checkin-needed
https://hg.mozilla.org/integration/b2g-inbound/rev/c3116313d9e1

In the future, please try to be more conscientious about what platforms and tests you run on your Try pushes (i.e. not running desktop/Android builds & tests on a B2G-only change). It saves a lot of resources and reduces the backlog felt by all developers when using Try. The link below gives some recommended practices if you need more assistance.
https://wiki.mozilla.org/Sheriffing/How:To:Recommended_Try_Practices
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/c3116313d9e1
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.0 S5 (4july)
Could you uplift to aurora branch?
Flags: needinfo?(pchangbin)
(In reply to Randy Lin [:rlin] from comment #21)
> Could you uplift to aurora branch?
I think, it's better be. But, I don't have enough permission for it.
Would you please?
Flags: needinfo?(pchangbin)
blocking-b2g: --- → 2.0?
Whiteboard: [webrtc-uplift]
h264 webrtc support is going to ship no matter what to 2.0, so I don't think this will be a blocking issue for 2.0. We can consider getting an approval on this patch to allow us to disable h264 on devices for testing.
blocking-b2g: 2.0? → backlog
(In reply to Jason Smith [:jsmith] from comment #23)
> h264 webrtc support is going to ship no matter what to 2.0, so I don't think
> this will be a blocking issue for 2.0. We can consider getting an approval
> on this patch to allow us to disable h264 on devices for testing.

I'll note that we've only targeted OMX H.264 for the Flame/8x10, and there's no guarantee it will work on other devices (and given the issues getting it to work, likely it won't on several/many of them without more work).

I was expecting we would tell vendors to turn h.264 *on* (using the pref) for supported handsets/chipsets.  Note that the pref is off today, and if the preference doesn't work, there's no way a vendor can simply turn off H.264 if it doesn't work in 2.0.
Flags: needinfo?(jsmith)
Okay, thanks for the clarification. I'll fix the flag here then.
blocking-b2g: backlog → 2.0+
Flags: needinfo?(jsmith)
Component: General → WebRTC
Product: Firefox OS → Core
Whiteboard: [webrtc-uplift]
You need to log in before you can comment on or make changes to this bug.