New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 614062 link

Starred by 4 users

Issue metadata

Status: Fixed
Owner:
Closed: May 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Chrome , Mac
Pri: 1
Type: Bug



Sign in to add a comment

AllowPepperMediaStreamAPI mismatch the whitelist in IsNaClAllowed, causing hangouts app not able to apply overlay to video

Project Member Reported by q...@google.com, May 23 2016

Issue description

UserAgent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/50.0.2661.102 Safari/537.36

Example URL:
https://hangouts.google.com/hangouts/_/?old

Steps to reproduce the problem:
1. Go to the specified URL with a Google+-enabled account
2. Join the created hangout
3. Go to the sidebar and click on the "Effects" app; if it's not present, hover over the three-dots button to bring up the apps drawer, and click on "Add", then find it in the resulting apps browser.
4. Once the Effects app is loaded, click on the "Randomize" button at the bottom: this should apply random image overlays to the self video

What is the expected behavior?
Local video should be shown with image overlays around the face of the local participant.

What went wrong?
Image overlays are not rendered.

If the hangout is hosted on talkgadget.google.com, for example, the image overlays would work.

Does it occur on multiple sites: No

Is it a problem with a plugin? Yes The video effects plugin for WebRTC

Did this work before? N/A 

Does this work in other browsers? Yes 

Chrome version: 50.0.2661.102  Channel: stable
OS Version: 
Flash Version: Shockwave Flash 21.0 r0

hangouts.google.com should have been completely whitelisted in https://codereview.chromium.org/1552383002 but only got whitelisted in IsNaClAllowed, and missed the whitelist in AllowPepperMediaStreamAPI.
 

Comment 1 by q...@google.com, May 23 2016

This issue has been fixed with https://codereview.chromium.org/1974413003/, and verified in Windows Chrome Version 53.0.2746.0 canary (64-bit). I would like to request a merge to M51 and M52. Thanks.

Comment 2 by q...@google.com, May 23 2016

The essence of the patch is just to add hangouts.google.com to the 2nd whitelist. It included a refactoring of whitelist management into a separate class on reviewers' demand. But the end result is still the same and the risk is low.

Comment 3 by q...@chromium.org, May 23 2016

Components: Platform>Apps>Hangouts Internals>Plugins>Pepper
Labels: -Pri-2 -Type-Compat Merge-Request-52 Merge-Request-51 OS-Chrome OS-Mac OS-Windows Pri-0 Type-Bug
Owner: q...@chromium.org

Comment 4 by q...@chromium.org, May 23 2016

Labels: -Pri-0 Pri-1
Status: Started (was: Unconfirmed)

Comment 5 by q...@chromium.org, May 23 2016

Committed: https://crrev.com/81dcbd6123178d58c5edc53d1453e03f9afa31ca
Cr-Commit-Position: refs/heads/master@{#395172}

Comment 6 by gov...@chromium.org, May 23 2016

Cc: sshruthi@chromium.org
Labels: -Merge-Request-51 Merge-Approved-51
Approving merge to M51 branch 2704 based on comment #1 and #2. 
If possible, could you please verify the fix on other OSs besides windows too. Once that is done, please merge to M51 branch 2704 before 5:00 PM PST today (Monday) in order to make it to M51 Desktop Stable candidate cut. Thank you.

 

Comment 7 by q...@chromium.org, May 23 2016

Also verified on Mac Chrome Version 53.0.2746.0 canary (64-bit).

Comment 8 Deleted

Comment 9 by gov...@chromium.org, May 23 2016

Please merge to M51 branch 2704 before 5:00 PM PST today (Monday) in order to make it to M51 Desktop Stable candidate cut. Thank you.
Project Member

Comment 10 by bugdroid1@chromium.org, May 23 2016

Labels: -merge-approved-51 merge-merged-2704
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8458f5b3e94c93b929bb47149213b29210983955

commit 8458f5b3e94c93b929bb47149213b29210983955
Author: qaz <qaz@google.com>
Date: Mon May 23 20:37:35 2016

Cherry-picking hangouts.google.com whitelisting change into M51.

Urgent: whitelist hangouts.google.com further (should've been done in issue 1552383002 from Dec 2015, but...) to allow video effects plugin access; tag on meet.google.com for a similar purpose; tax: refactor whitelist checks into separate thing.

R=sshruthi@chromium.org,govind@chromium.org,sky@chromium.org,tsepez@chromium.org
BUG= 614062 
NOTRY=true
NOPRESUBMIT=true

Review-Url: https://codereview.chromium.org/1974413003
Cr-Commit-Position: refs/heads/master@{#395172}
(cherry picked from commit 81dcbd6123178d58c5edc53d1453e03f9afa31ca)

Review-Url: https://codereview.chromium.org/2001233002
Cr-Commit-Position: refs/branch-heads/2704@{#644}
Cr-Branched-From: 6e53600def8f60d8c632fadc70d7c1939ccea347-refs/heads/master@{#386251}

[modify] https://crrev.com/8458f5b3e94c93b929bb47149213b29210983955/chrome/chrome_renderer.gypi
[modify] https://crrev.com/8458f5b3e94c93b929bb47149213b29210983955/chrome/chrome_tests_unit.gypi
[modify] https://crrev.com/8458f5b3e94c93b929bb47149213b29210983955/chrome/renderer/OWNERS
[add] https://crrev.com/8458f5b3e94c93b929bb47149213b29210983955/chrome/renderer/app_categorizer.cc
[add] https://crrev.com/8458f5b3e94c93b929bb47149213b29210983955/chrome/renderer/app_categorizer.h
[add] https://crrev.com/8458f5b3e94c93b929bb47149213b29210983955/chrome/renderer/app_categorizer_unittest.cc
[modify] https://crrev.com/8458f5b3e94c93b929bb47149213b29210983955/chrome/renderer/chrome_content_renderer_client.cc
[modify] https://crrev.com/8458f5b3e94c93b929bb47149213b29210983955/chrome/renderer/chrome_content_renderer_client_unittest.cc

Comment 11 by tin...@google.com, May 24 2016

Labels: -Merge-Request-52 Merge-Approved-52 Hotlist-Merge-Approved
Your change meets the bar and is auto-approved for M52 (branch: 2743)
Cc: -sshruthi@chromium.org
Project Member

Comment 13 by bugdroid1@chromium.org, May 24 2016

Labels: -merge-approved-52 merge-merged-2743
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0901e142877ed60d6522dabb8b3f72275a86e5a1

commit 0901e142877ed60d6522dabb8b3f72275a86e5a1
Author: qaz <qaz@google.com>
Date: Tue May 24 21:51:15 2016

Cherry-picking hangouts.google.com whitelisting change into M52.

Urgent: whitelist hangouts.google.com further (should've been done in issue 1552383002 from Dec 2015, but...) to allow video effects plugin access; tag on meet.google.com for a similar purpose; tax: refactor whitelist checks into separate thing.

R=sky@chromium.org,tsepez@chromium.org
BUG= 614062 
NOTRY=true
NOPRESUBMIT=true

Review-Url: https://codereview.chromium.org/1974413003
Cr-Commit-Position: refs/heads/master@{#395172}
(cherry picked from commit 81dcbd6123178d58c5edc53d1453e03f9afa31ca)

Review-Url: https://codereview.chromium.org/2011623002
Cr-Commit-Position: refs/branch-heads/2743@{#42}
Cr-Branched-From: 2b3ae3b8090361f8af5a611712fc1a5ab2de53cb-refs/heads/master@{#394939}

[modify] https://crrev.com/0901e142877ed60d6522dabb8b3f72275a86e5a1/chrome/chrome_renderer.gypi
[modify] https://crrev.com/0901e142877ed60d6522dabb8b3f72275a86e5a1/chrome/chrome_tests_unit.gypi
[modify] https://crrev.com/0901e142877ed60d6522dabb8b3f72275a86e5a1/chrome/renderer/OWNERS
[add] https://crrev.com/0901e142877ed60d6522dabb8b3f72275a86e5a1/chrome/renderer/app_categorizer.cc
[add] https://crrev.com/0901e142877ed60d6522dabb8b3f72275a86e5a1/chrome/renderer/app_categorizer.h
[add] https://crrev.com/0901e142877ed60d6522dabb8b3f72275a86e5a1/chrome/renderer/app_categorizer_unittest.cc
[modify] https://crrev.com/0901e142877ed60d6522dabb8b3f72275a86e5a1/chrome/renderer/chrome_content_renderer_client.cc
[modify] https://crrev.com/0901e142877ed60d6522dabb8b3f72275a86e5a1/chrome/renderer/chrome_content_renderer_client_unittest.cc

Cc: pbomm...@chromium.org ranjitkan@chromium.org
Labels: TE-NeedsTriageFromMTV
Verified the above issue on Windows & Mac 10.11.5 with chrome version '51.0.2704.63' BETA build and Effect overlays gets added on when Randomize is clicked. 


Note: Not verified on Linux as it requires 2 or more linux laptop with camera, request MTV to verify the same.

Comment 15 by q...@chromium.org, May 25 2016

Status: Fixed (was: Started)

Sign in to add a comment