Closed Bug 1173255 Opened 9 years ago Closed 9 years ago

Cleanup MediaManager e10s code (avoids opening it on clear cookies) in prep for deviceId constraint

Categories

(Core :: WebRTC, defect, P2)

Product:
Core
Component:
WebRTC
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla41
Tracking Flags:
Tracking Status
e10s + ---
firefox41 --- fixed
Blocking Flags:
backlog webrtc/webaudio+

People

(Reporter: jib, Assigned: jib)

References

Details

Attachments

(6 files, 6 obsolete files)

MozReview Request: Bug 1173255 - move facingMode constraint. r=jesup
40 bytes, text/x-review-board-request
Details
MozReview Request: Bug 1173255 - move PMedia from PBackground to PContent. r=jesup
40 bytes, text/x-review-board-request
Details
MozReview Request: Bug 1173255 - refactor getOriginKey to main
40 bytes, text/x-review-board-request
Details
MozReview Request: Bug 1173255 - cleanup runnables and task in MediaManager
40 bytes, text/x-review-board-request
Details
MozReview Request: Bug 1173255 - compensate for PContent being useless in non-e10s
40 bytes, text/x-review-board-request
Details
MozReview Request: Bug 1173255 - incorporate feedback. r=jesup
40 bytes, text/x-review-board-request
jesup
: review+
Details 
Decided to separate out this chunk of work from Bug 1037389.

The raw deviceId is a simple property. The problem is we need to operate not on the raw ids but on the same origin-unique ids the content script is using (from enumerateDevices), which is harder.

This is some needed cleanup in preparation for the deviceId constraint implementation:

 0 A cleanupfacingmode: deviceId prep + move facingMode constraint
 1 A pcontent.patch: move PMedia from PBackground to PContent
 2 A omain: refactor getOriginKey to main
 3 A newtaskfrom: cleanup runnables and task in MediaManager
 4 A none10s: compensate for PContent being useless in non-e10s

This means a lot less thread-hopping (in both content and chrome process) for enumerateDevices which was getting unwieldy. This is important because enumerateDevices will play a role internally in getUserMedia from now on, now that origin-unique ids are needed to resolve potential deviceId constraints.

Refactored the relevant MediaManager runnables to use lambdas, to keep better track of the extra dispatches, and to allow using media::Pledges effectively to avoid passing DOM objects off-main-thread (or now off-process).

A side-benefit of this cleanup is that MediaManager + thread should no longer be opened by the act of clearing cookies, which if you run with "Clear cookies on shutdown" is on every startup and shutdown. 

Things seem to be working as well as before locally, though I'm sure there may be kinks on try, but I think this is ready for review.
Bug 1173255 - deviceId prep + move facingMode constraint
Attachment #8617757 - Flags: review?(rjesup)
Bug 1173255 - move PMedia from PBackground to PContent
Attachment #8617758 - Flags: review?(rjesup)
Bug 1173255 - refactor getOriginKey to main
Attachment #8617759 - Flags: review?(rjesup)
Bug 1173255 - cleanup runnables and task in MediaManager
Attachment #8617760 - Flags: review?(rjesup)
Bug 1173255 - compensate for PContent being useless in non-e10s
Attachment #8617761 - Flags: review?(rjesup)
Discussed with botond on irc: Some of the try problems are from Bug 1170388 which is being reopened.
Depends on: 1170388
backlog: --- → webRTC+
Rank: 21
Comment on attachment 8617757 [details]
MozReview Request: Bug 1173255 - move facingMode constraint. r=jesup

https://reviewboard.mozilla.org/r/10715/#review9493

::: dom/media/webrtc/MediaEngineCameraVideoSource.h:89
(Diff revision 1)
> +  static uint32_t FitnessDistance(nsString aN,

I note this is "aN" and the other FitnessDistance()es above use "n".  Minor; prefer to see them match

::: dom/webidl/MediaTrackConstraintSet.webidl:1
(Diff revision 1)
>  /* -*- Mode: IDL; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */

Need a DOM peer review for webidl files
Attachment #8617757 - Flags: review?(rjesup) → review+
Comment on attachment 8617758 [details]
MozReview Request: Bug 1173255 - move PMedia from PBackground to PContent. r=jesup

https://reviewboard.mozilla.org/r/10717/#review9497

r+ing since the sChild issue is pre-existing; we may want a followup on it

::: dom/media/MediaManager.cpp:1489
(Diff revision 1)
> +  media::Child::Get();

Add a comment

::: dom/media/systemservices/MediaChild.cpp:40
(Diff revision 1)
> +static Child* sChild;

Raw static non-refcounted pointer?  Is Child refcounted?  Will this leak?  What's the threading requirements on sChild?  And is there a reason it's a global static, not a class static?

(Likely this is largely managed by the IPC calls.)

At least needs some comments and maybe thread asserts on accessors; may need more.

And yes, I realize you just moved it. :-)
Attachment #8617758 - Flags: review?(rjesup) → review+
Comment on attachment 8617759 [details]
MozReview Request: Bug 1173255 - refactor getOriginKey to main

https://reviewboard.mozilla.org/r/10719/#review9501

Presuming that's an oops

::: dom/media/MediaManager.cpp:1434
(Diff revision 1)
> +    if (true) {

I think this was an oops....
Attachment #8617759 - Flags: review?(rjesup) → review+
Comment on attachment 8617760 [details]
MozReview Request: Bug 1173255 - cleanup runnables and task in MediaManager

https://reviewboard.mozilla.org/r/10721/#review9503

::: dom/media/MediaManager.cpp:1280
(Diff revision 1)
> -    if (true) {
> +      if (!mgr) {

I see this patch fixes that

::: dom/media/MediaManager.cpp:1905
(Diff revision 1)
> + 

trailing space

::: dom/media/systemservices/MediaUtils.h:70
(Diff revision 1)
> +    class F : public FunctorsBase

'F' is kinda non-descriptive - is there a better name?
Attachment #8617760 - Flags: review?(rjesup) → review+
Attachment #8617761 - Flags: review?(rjesup) → review+
Comment on attachment 8617761 [details]
MozReview Request: Bug 1173255 - compensate for PContent being useless in non-e10s

https://reviewboard.mozilla.org/r/10723/#review9505

Ship It!
(In reply to Randell Jesup [:jesup] from comment #7)
> ::: dom/webidl/MediaTrackConstraintSet.webidl:1
> (Diff revision 1)
> >  /* -*- Mode: IDL; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> 
> Need a DOM peer review for webidl files

Actually, let me remove those changes as they're not needed yet. That should take care of the extra review requirement.
https://reviewboard.mozilla.org/r/10717/#review9525

> Add a comment

This may be redundant as well now that I think about it. Removing the line instead.

> Raw static non-refcounted pointer?  Is Child refcounted?  Will this leak?  What's the threading requirements on sChild?  And is there a reason it's a global static, not a class static?
> 
> (Likely this is largely managed by the IPC calls.)
> 
> At least needs some comments and maybe thread asserts on accessors; may need more.
> 
> And yes, I realize you just moved it. :-)

Child is not ref-counted and does not leak, because IPC (in this case PContent) handles lifetimes for actors, and does so without ref-counting. [1]

While using ref-counting is an option, as described a bit down in [1] - and I took that approach for MediaParent because I needed it there - it adds complexity, so I didn't do that for MediaChild.

PContent is main-thread-only is what I've been told (and matches my observations). I'll throw in a MOZ_ASSERT.

The pattern seems common to me [2] e.g. [3] is not even static. No reason it can't be class static, except it seems redundant given it's already in the media:: namespace.


[1] https://developer.mozilla.org/en-US/docs/IPDL/Tutorial#Subprotocol_Actor_Lifetime
[2] http://mxr.mozilla.org/mozilla-central/search?string=Child-%3ESend
[3] http://mxr.mozilla.org/mozilla-central/source/netwerk/ipc/NeckoChild.cpp?rev=3182e5961729#38
Bug 1173255 - move facingMode constraint. r=jesup
Bug 1173255 - move PMedia from PBackground to PContent. r=jesup
Bug 1173255 - refactor getOriginKey to main
Bug 1173255 - cleanup runnables and task in MediaManager
Bug 1173255 - compensate for PContent being useless in non-e10s
Bug 1173255 - incorporate feedback. r=jesup
Attachment #8621913 - Flags: review?(rjesup)
Review board hosed my review series (Bug 1174353) so I had to submit afresh using a separate reviewid.

I figure this still beats splinter with all the whitespace changes.

/r/11083 - Bug 1173255 - move facingMode constraint. r=jesup
/r/11085 - Bug 1173255 - move PMedia from PBackground to PContent. r=jesup
/r/11087 - Bug 1173255 - refactor getOriginKey to main
/r/11089 - Bug 1173255 - cleanup runnables and task in MediaManager
/r/11091 - Bug 1173255 - compensate for PContent being useless in non-e10s
/r/11093 - Bug 1173255 - incorporate feedback. r=jesup

Basically I made changes where you see r= above (the first two and the last)

I edited the first two patches (to avoid touching the webidl file, and some other reason).

I put all other changes (review feedback and try fixes) in the last patch.

I would like a review on the last patch even though I marked it r= (my mistake).
Attachment #8617757 - Attachment is obsolete: true
Attachment #8617758 - Attachment is obsolete: true
Attachment #8617759 - Attachment is obsolete: true
Attachment #8617760 - Attachment is obsolete: true
Attachment #8617761 - Attachment is obsolete: true
https://reviewboard.mozilla.org/r/11093/#review9685

::: dom/media/tests/mochitest/test_getUserMedia_constraints.html
(Diff revision 1)
> -  { message: "video overconstrained by facingMode fails",
> -    constraints: { video: { facingMode:{ exact: 'left' } },
> -                   fake: true },
> -    error: "NotFoundError" },
> -  { message: "video overconstrained by facingMode array fails",
> -    constraints: { video: { facingMode:{ exact: ['left', 'right'] } },
> -                   fake: true },
> -    error: "NotFoundError" },

I had to remove these tests that failed.

The reason these tests no longer work is that the facingMode constraint checking code has moved from MediaManager.cpp to down to webrtc/MediaEngineCameraVideoSource.cpp to join the other constraints. This means that the "fake" devices no longer respond to facingMode (which was a little weird to begin with, since it was the only constraint they implemented).
smacleod force-pushed my broken review series which seems to have fixed it, yay! So lets use that.

I'll try to remove the new redundant one.
OK so good news is the diffs are available in bz://1173255/jib but I wouldn't touch anything there, or you'll likely end up blocked by a busted draft like I am and have to log out of Review board to see the diffs.

We should therefore probably put further review comments in bz://1173255/jib2
Attachment #8617757 - Attachment description: MozReview Request: Bug 1173255 - deviceId prep + move facingMode constraint → MozReview Request: Bug 1173255 - move facingMode constraint. r=jesup
Attachment #8617757 - Attachment is obsolete: false
Attachment #8617757 - Flags: review+
Comment on attachment 8617757 [details]
MozReview Request: Bug 1173255 - move facingMode constraint. r=jesup

Bug 1173255 - move facingMode constraint. r=jesup
Comment on attachment 8617758 [details]
MozReview Request: Bug 1173255 - move PMedia from PBackground to PContent. r=jesup

Bug 1173255 - move PMedia from PBackground to PContent. r=jesup
Attachment #8617758 - Attachment description: MozReview Request: Bug 1173255 - move PMedia from PBackground to PContent → MozReview Request: Bug 1173255 - move PMedia from PBackground to PContent. r=jesup
Attachment #8617758 - Attachment is obsolete: false
Attachment #8617758 - Flags: review+
Comment on attachment 8617759 [details]
MozReview Request: Bug 1173255 - refactor getOriginKey to main

Bug 1173255 - refactor getOriginKey to main
Attachment #8617759 - Attachment is obsolete: false
Attachment #8617759 - Flags: review+
Comment on attachment 8617760 [details]
MozReview Request: Bug 1173255 - cleanup runnables and task in MediaManager

Bug 1173255 - cleanup runnables and task in MediaManager
Attachment #8617760 - Attachment is obsolete: false
Attachment #8617760 - Flags: review+
Comment on attachment 8617761 [details]
MozReview Request: Bug 1173255 - compensate for PContent being useless in non-e10s

Bug 1173255 - compensate for PContent being useless in non-e10s
Attachment #8617761 - Attachment is obsolete: false
Attachment #8617761 - Flags: review+
Bug 1173255 - incorporate feedback. r=jesup
Attachment #8622792 - Flags: review?(rjesup)
Attachment #8621908 - Attachment is obsolete: true
Attachment #8621909 - Attachment is obsolete: true
Attachment #8621910 - Attachment is obsolete: true
Attachment #8621911 - Attachment is obsolete: true
Attachment #8621912 - Attachment is obsolete: true
Attachment #8621913 - Attachment is obsolete: true
Attachment #8621913 - Flags: review?(rjesup)
Review Board has been fixed (Bug 1174353) so updated review request. Still chasing a leak.
The UI of reviewboard is confusing - what exactly am I supposed to be reviewing?  Is it the "incorporate feedback" patch?  If so, is it both revisions, or just the last one?
Flags: needinfo?(jib)
Comment on attachment 8622792 [details]
MozReview Request: Bug 1173255 - incorporate feedback. r=jesup

https://reviewboard.mozilla.org/r/11005/#review9819

Ship It!
Attachment #8622792 - Flags: review?(rjesup) → review+
(In reply to Randell Jesup [:jesup] from comment #32)
> The UI of reviewboard is confusing - what exactly am I supposed to be
> reviewing?

Review board lets you cut the cake both ways. It treats the 6 patches individually, but also keeps track of changes between pushes to mozReview (Rev 1 vs 2).

> Is it the "incorporate feedback" patch?  If so, is it both
> revisions, or just the last one?

Yes, I put new changes in that patch. Both revisions: https://reviewboard.mozilla.org/r/11005/diff/2

But I also updated the first two patches from feedback (see comment 20 and comment 21):

https://reviewboard.mozilla.org/r/10715/diff/1-2
https://reviewboard.mozilla.org/r/10717/diff/1-2

Note that the second diff pulls in stuff from being rebased. I just removed media::Child::Get();

Reference:

/r/10715 - Bug 1173255 - move facingMode constraint. r=jesup
/r/10717 - Bug 1173255 - move PMedia from PBackground to PContent. r=jesup
/r/10719 - Bug 1173255 - refactor getOriginKey to main
/r/10721 - Bug 1173255 - cleanup runnables and task in MediaManager
/r/10723 - Bug 1173255 - compensate for PContent being useless in non-e10s
/r/11005 - Bug 1173255 - incorporate feedback. r=jesup
Flags: needinfo?(jib)
Comment on attachment 8617757 [details]
MozReview Request: Bug 1173255 - move facingMode constraint. r=jesup

Bug 1173255 - move facingMode constraint. r=jesup
Comment on attachment 8617758 [details]
MozReview Request: Bug 1173255 - move PMedia from PBackground to PContent. r=jesup

Bug 1173255 - move PMedia from PBackground to PContent. r=jesup
Comment on attachment 8617759 [details]
MozReview Request: Bug 1173255 - refactor getOriginKey to main

Bug 1173255 - refactor getOriginKey to main
Comment on attachment 8617760 [details]
MozReview Request: Bug 1173255 - cleanup runnables and task in MediaManager

Bug 1173255 - cleanup runnables and task in MediaManager
Comment on attachment 8617761 [details]
MozReview Request: Bug 1173255 - compensate for PContent being useless in non-e10s

Bug 1173255 - compensate for PContent being useless in non-e10s
Comment on attachment 8622792 [details]
MozReview Request: Bug 1173255 - incorporate feedback. r=jesup

Bug 1173255 - incorporate feedback. r=jesup
Attachment #8622792 - Flags: review+ → review?(rjesup)
Comment on attachment 8622792 [details]
MozReview Request: Bug 1173255 - incorporate feedback. r=jesup

Reviewboard didn't give me the option to Ship It, and when I published the review it game me "Something went wrong"... so doing it manually
Attachment #8622792 - Flags: review?(rjesup) → review+
Comment on attachment 8617759 [details]
MozReview Request: Bug 1173255 - refactor getOriginKey to main

Bug 1173255 - refactor getOriginKey to main
Comment on attachment 8617760 [details]
MozReview Request: Bug 1173255 - cleanup runnables and task in MediaManager

Bug 1173255 - cleanup runnables and task in MediaManager
Comment on attachment 8617761 [details]
MozReview Request: Bug 1173255 - compensate for PContent being useless in non-e10s

Bug 1173255 - compensate for PContent being useless in non-e10s
Comment on attachment 8622792 [details]
MozReview Request: Bug 1173255 - incorporate feedback. r=jesup

Bug 1173255 - incorporate feedback. r=jesup
Attachment #8622792 - Flags: review+ → review?(rjesup)
Leak fixed. Embarrassing.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=34ecd9839f6b

Doing a full try (in review board) just in case.
Comment on attachment 8617757 [details]
MozReview Request: Bug 1173255 - move facingMode constraint. r=jesup

Bug 1173255 - move facingMode constraint. r=jesup
Comment on attachment 8617758 [details]
MozReview Request: Bug 1173255 - move PMedia from PBackground to PContent. r=jesup

Bug 1173255 - move PMedia from PBackground to PContent. r=jesup
Comment on attachment 8617759 [details]
MozReview Request: Bug 1173255 - refactor getOriginKey to main

Bug 1173255 - refactor getOriginKey to main
Comment on attachment 8617760 [details]
MozReview Request: Bug 1173255 - cleanup runnables and task in MediaManager

Bug 1173255 - cleanup runnables and task in MediaManager
Comment on attachment 8617761 [details]
MozReview Request: Bug 1173255 - compensate for PContent being useless in non-e10s

Bug 1173255 - compensate for PContent being useless in non-e10s
Comment on attachment 8622792 [details]
MozReview Request: Bug 1173255 - incorporate feedback. r=jesup

Bug 1173255 - incorporate feedback. r=jesup
Rebased.
Comment on attachment 8622792 [details]
MozReview Request: Bug 1173255 - incorporate feedback. r=jesup

Of course it didn't give me the option to r+ it, so doing it manually
Attachment #8622792 - Flags: review?(rjesup) → review+
No longer depends on: 1170388
This patch added a new method in MediaChild.h which was correctly annotated as 'override'. However, the same class had a preexisting method that could (should) have been annotated as 'override' but was not yet. When clang 3.6 & newer sees inconsistent override usage within a class, it warns, and warnings are fatal in this directory, so this caused local build bustage for anyone with clang 3.6+.

I pushed a followup to add the missing 'override' keyword, with blanket r+ that ehsan granted me for fixes of this sort over in bug 1126447 comment 2:
https://hg.mozilla.org/integration/mozilla-inbound/rev/edbf66bef7d7
>  so this caused local build bustage for anyone with clang 3.6+.
(er, a local build warning, which is bustage if you're building with --enable-warnings-as-errors)
Thanks Daniel!
https://hg.mozilla.org/mozilla-central/rev/53063473eb54
https://hg.mozilla.org/mozilla-central/rev/edbf66bef7d7
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox41: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: