Affected versions]: Tested on Nightly 60.0a1(2018-02-06) [Affected platforms]: Tested on Mac OS X 10.12 [Steps to reproduce]: Prerequisites: 1. Make sure you have 1 camera connected to your system. STR: 1. Open Firefox and go to this test page: https://jsfiddle.net/pehrsons/y2rvuqdL/4/ 2. Click on "Check cameras" 3. Allow your cameras to start to capture 3. Unplug the camera [Expected result]: The status of the unplug camera should be "status: ended" [Actual result]: The status of the unplug camera is "status: live" NOTE: This is a regression and I found out the regression range: Last good revision: bbac8c59d58c50fec337b4832ad8ab28bc507bee 39:30.61 INFO: First bad revision: f2ce61d1a59297b7ad3c91496196fa3a0b776aa7 39:30.61 INFO: Pushlog: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=bbac8c59d58c50fec337b4832ad8ab28bc507bee&tochange=f2ce61d1a59297b7ad3c91496196fa3a0b776aa7 Dan, can you please take a look at this? Thanks

New regression in 60 for mac.

This is basically the same problem as Bug 1374938, the BUILD.gn files used webrtc.org's avfoundation wrapper under the folder video_capture/objc instead of our own implementation under video_capture/mac. We should be able to upstream our BUILD.gn changes to webrtc.org and not have this problem in the future. In the event I get some pushback about this (they were hesitant to accept changes that point to files that don't exist in their repository the last time I upstreamed) another possibility would be to add some kind of compile time check that will cause the build to fail if we're building the wrong set of files. I'm not certain why we have our own version of this code. Perhaps it is something they would be willing to accept upstream, but that seems like a much larger task.

Looking at code history for one of the files involved it looks like we're using an old version of the mac capture code from upstream [1]. Meanwhile the current upstream mac/ios impl seems quite new [2]. IMO we should look at carrying over any changes we made to the old impl to the new one. Perhaps the changes will then make sense to upstream. [1] https://hg.mozilla.org/mozilla-central/log/tip/media/webrtc/trunk/webrtc/modules/video_capture/mac/video_capture_mac.mm [2] https://hg.mozilla.org/mozilla-central/loga/tip/media/webrtc/trunk/webrtc/modules/video_capture/objc/video_capture.mm

tracking as new regression.

(In reply to Andreas Pehrson [:pehrsons] from comment #3) > Looking at code history for one of the files involved it looks like we're > using an old version of the mac capture code from upstream [1]. Meanwhile > the current upstream mac/ios impl seems quite new [2]. > > IMO we should look at carrying over any changes we made to the old impl to > the new one. Perhaps the changes will then make sense to upstream. > I agree. The main difference seems to be the addition of the device change handler. I have that working now, I'll double check to see if we made any other significant changes.

The only other change which seems significant are the additional FPS settings made available in Bug 1273734. To be honest, I'm not sure if we gain much by diverging from the webrtc.org code here and I'd prefer to drop the changes rather than continue to maintain them. I'm open to addressing them in a follow up bug if we think they are important enough to keep.

Try job here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=aa40a3e1ee887e646454140780059ea06a544324

Thanks Dan. Sorry, I'm really not confident with this code, it being obj-c and all. I'm forwarding to jib as he did the original reviews.

Dan, considering that I just came across a different issue (bug 1438554) from using the newer version should we think about reverting? And maybe plan to switch over for a future release.

Yes, I agree. I filed Bug 1439997 to handle the switch over. I have a patch to do the revert already made, I'll put it up for review here in a few minutes.

Comment on attachment 8950603 [details] Bug 1436959 - Update generated files; https://reviewboard.mozilla.org/r/219876/#review228138 rs=me (too long to reasonably review in detail, and much of it (most) isn't really "changes")

Comment on attachment 8950602 [details] Bug 1436959 - Use correct avfoundation library in video_capture; https://reviewboard.mozilla.org/r/219874/#review228140

(In reply to Randell Jesup [:jesup] from comment #15) > Comment on attachment 8950603 [details] > Bug 1436959 - Update generated files; > > https://reviewboard.mozilla.org/r/219876/#review228138 > > rs=me (too long to reasonably review in detail, and much of it (most) isn't > really "changes") I was surprised and a little dismayed at how much churn there was in the generated files considering how small the change to the BUILD.gn file was. Chris Manchester and I discussed stripping unused items from the generated json files, based on this it is looking like that is something we will want sooner rather than later.

Chris, do you mind having a quick look at this try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c8f70dc2d0b8b011d7c827dda02d099bf9ce1f84 It looks like we're generating moz.build files that are not properly sorted on Windows. The changes I made should have affected OS X only. My apologies if it's something I've done wrong :)

I have a patch I'll post here shortly. Sorry about that, I incorrectly wasn't using the case-insensitive sorted iterator for moz.build lists.

Comment on attachment 8953204 [details] Bug 1436959 - Use the correct case-insensitive sorted iterator when sorting lists in generated GN moz.build files. https://reviewboard.mozilla.org/r/222496/#review228636 LGTM, thanks! I'll land this with my patches.

Pushed by dminor@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/7b7a710fdf77 Use the correct case-insensitive sorted iterator when sorting lists in generated GN moz.build files. r=dminor https://hg.mozilla.org/integration/mozilla-inbound/rev/8ea48542a4b6 Use correct avfoundation library in video_capture; r=jesup https://hg.mozilla.org/integration/mozilla-inbound/rev/ed6b43fa64c9 Update generated files; r=jesup

https://hg.mozilla.org/mozilla-central/rev/7b7a710fdf77 https://hg.mozilla.org/mozilla-central/rev/8ea48542a4b6 https://hg.mozilla.org/mozilla-central/rev/ed6b43fa64c9

I verified this issue on Mac OS X 10.12 with FF Nightly 60.0a1(2018-02-25) using the steps from the description and I can confirm the fix. I will mark this as verified fixed.