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

Status: RESOLVED FIXED

(Core :: WebRTC, defect)

Description

[Changbin Park]

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.

Comment 1

[Changbin Park]

Attachment: Perform reserve omx codec only when preference "media.peerconnection.video.h264_enabled" is set to true.

Return VCM_CODEC_RESOURCE_H264 from VcmSIPCCBinding::getVideoCodecsHw() makes H.264 working.
This makes the preference is checked before using OMX codec.

Comment 2

[Ethan Hugg [:ehugg]]

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.

Comment 3

[Ryan VanderMeulen [:RyanVM]]

Can we please run this through Try to at least confirm that it builds?

Comment 4

[Ryan VanderMeulen [:RyanVM]]

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

Comment 5

[Changbin Park]

(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.

Comment 6

[Ryan VanderMeulen [: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.

Comment 7

[Changbin Park]

Attachment: [hg-patch] Perform reserve omx codec only when preference "media.peerconnection.video.h264_enabled" is set to true.

I'm attaching hg-patch instead of git-patch.
It's converted by git-patch-to-hg-patch in moz-git-tools

Comment 8

[Changbin Park]

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 <[e-mail]>
>
>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
>

Comment 9

[Changbin Park]

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 <[e-mail]>
>
>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
>

Comment 10

[Changbin Park]

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 <[e-mail]>

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

Comment 11

[Changbin Park]

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 <[e-mail]>

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

Comment 12

[Changbin Park]

(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.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Done :)

Comment 14

[Ethan Hugg [:ehugg]]

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.

Comment 15

[Ethan Hugg [:ehugg]]

Attachment: 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(-)

Comment 16

[Ethan Hugg [:ehugg]]

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

Comment 17

[Changbin Park]

Thank you very much Ethan. You're very thoughtful...
I'm still getting level 1 access account.

Comment 18

[Changbin Park]

It seems no problem with build.
https://tbpl.mozilla.org/?tree=Try&rev=cd0e82f1f7d0

Comment 19

[Ryan VanderMeulen [:RyanVM]]

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

Comment 20

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/c3116313d9e1

Comment 21

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Could you uplift to aurora branch?

Comment 22

[Changbin Park]

(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?

Comment 23

[Jason Smith [:jsmith]]

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.

Comment 24

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

(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.

Comment 25

[Jason Smith [:jsmith]]

Okay, thanks for the clarification. I'll fix the flag here then.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/34ef96fed1e1