1436959 - Devicechange event is not changed if you unplug a camera

Status: VERIFIED FIXED

(Core :: WebRTC: Audio/Video, defect, P2)

Description

[ovidiu boca[:Ovidiu]]

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

Comment 1

[Andreas Pehrson [:pehrsons]]

New regression in 60 for mac.

Comment 2

[Dan Minor [:dminor]]

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.

Comment 3

[Andreas Pehrson [:pehrsons]]

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

Comment 4

[Julien Cristau [:jcristau]]

tracking as new regression.

Comment 5

[Dan Minor [:dminor]]

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

Comment 6

[Dan Minor [:dminor]]

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.

Comment 7

[Dan Minor [:dminor]]

Attachment: Bug 1436959 - Use correct avfoundation library in video_capture;

Bug 1286429 implemented the ondevicechange event for OS X. Those patches
applied to an older version of the OS X platform specific video capture
code. The Branch 57 update which landed in Bug 1341285 brought in a new
version of the capture code. This broke the ondevicechange event. This was
fixed in Bug 1374938, but that fix involved restoring the old version of
the code rather than updating the new version to support the event.

This patch applies the ondevicechange event support to the latest version
of the OS X video capture code. The largest difference is that the new
code uses ARC and so some explicit memory management can be avoided.

Review commit: https://reviewboard.mozilla.org/r/219874/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/219874/

Comment 8

[Dan Minor [:dminor]]

Attachment: Bug 1436959 - Update generated files;

Review commit: https://reviewboard.mozilla.org/r/219876/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/219876/

Comment 9

[Dan Minor [:dminor]]

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

Comment 10

[Andreas Pehrson [:pehrsons]]

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.

Comment 11

[Liz Henry (:lizzard) (relman/hg->git project)]

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.

Comment 12

[Dan Minor [:dminor]]

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 13

[Dan Minor [:dminor]]

Comment on attachment 8950602 [details]
Bug 1436959 - Use correct avfoundation library in video_capture;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/219874/diff/1-2/

Comment 14

[Dan Minor [:dminor]]

Comment on attachment 8950603 [details]
Bug 1436959 - Update generated files;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/219876/diff/1-2/

Comment 15

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

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 16

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

Comment on attachment 8950602 [details]
Bug 1436959 - Use correct avfoundation library in video_capture;

https://reviewboard.mozilla.org/r/219874/#review228140

Comment 17

[Dan Minor [:dminor]]

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

Comment 18

[Dan Minor [:dminor]]

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

Comment 19

[Chris Manchester (limited bugmail, email directly)]

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 20

[Chris Manchester (limited bugmail, email directly)]

Attachment: Bug 1436959 - Use the correct case-insensitive sorted iterator when sorting lists in generated GN moz.build files.

Review commit: https://reviewboard.mozilla.org/r/222496/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/222496/

Comment 21

[Dan Minor [:dminor]]

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.

Comment 22

[Pulsebot]

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

Comment 23

[Natalia Csoregi [:nataliaCs]]

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

Comment 24

[ovidiu boca[:Ovidiu]]

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.