Bug 175472

Summary: WebRTC video does not resume receiving when switching back to Safari 11 on iOS
Product: WebKit Reporter: Andrew Morris <andrew>
Component: WebRTCAssignee: youenn fablet <youennf>
Status: RESOLVED FIXED    
Severity: Normal CC: andj2223, buildbot, cdumez, commit-queue, darin, eric.carlson, jer.noble, jonlee, msander, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: Other   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
sysdiagnose
none
screen-recording-of-repro
none
switch-camera-icon
none
video-enabled-icon
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Andrew Morris 2017-08-10 20:14:53 PDT
1. Join the same room on https://safari.opentokrtc.com on two devices, one that is iOS Safari, the other doesn't matter
2. Wait for both sides to send and receive video
3. On iOS, switch apps so Safari is in the background
4. Switch back to Safari

Result: Remote video is frozen or black
Expected: Remote video should resume flowing

I have tried calling .play() on the video to no avail. This also happens P2P, and I suspect that it is unrelated to the OpenTok stack, but I can produce a minimal repro app if needed. The tricky bit in setting this up though is that you must use multiple devices. I have done a similar test at https://webrtc.github.io/samples/src/content/peerconnection/pc1/ but video resumes flowing correctly, and I suspect this is because the video being sent is being affected by the same device.

Tested on iPhone with iOS beta 4 and iPad with iOS beta 5.
Comment 1 Radar WebKit Bug Importer 2017-08-11 20:14:53 PDT
<rdar://problem/33860863>
Comment 2 youenn fablet 2017-08-12 13:19:25 PDT
I tried reproducing this issue and was in most cases unable to do so.
I was able to reproduce it once but reloading the other page (not the iOS device one) fixed the issue so this probably does not count.

Some sysdiagnose might be useful since the issue might be below WebKit.
Comment 3 Andrew Morris 2017-08-14 18:57:21 PDT
@youenn Hmm. Are you using an internal build that's newer than beta 5? If not that would be strange because I was getting this reliably across multiple devices.
Comment 4 Andrew Morris 2017-08-15 18:08:27 PDT
Created attachment 318209 [details]
sysdiagnose
Comment 5 Andrew Morris 2017-08-15 18:19:02 PDT
Sysdiagnose attached

2017-08-16 UTC+10 AEST
10:54:10 - switch apps
10:54:15 - switch back to safari; remote video frozen
10:54:18 - siri activated
10:54:20 - siri dismissed; remote video black (local video did not go black this time)
Comment 6 youenn fablet 2017-08-15 21:22:06 PDT
It seems that the decoder has some issues and mediaserverd is returning kVTVideoDecoderMalfunctionErr as soon as WebKit is backgrounded.
And it does not seem to be able to recover.

In opentokrtc SFU, how often are key frames being sent by default?
Is this something you would be able to play to check whether that would have an impact?
Comment 7 Andrew Morris 2017-08-15 22:12:40 PDT
@youenn. I believe it's once per minute. I'll inquire about experimenting with this for you. However, please note this also occurred for P2P.
Comment 8 youenn fablet 2017-08-15 22:54:38 PDT
(In reply to Andrew Morris from comment #7)
> @youenn. I believe it's once per minute. I'll inquire about experimenting
> with this for you. However, please note this also occurred for P2P.

Before doing that, can you do a quick test: try to restart playing the black video by calling play() when coming back from Siri.
It might be that we are not restarting properly the autoplayed video that got paused due to backgrounding.

In that case, a temporary workaround would be to catch page visibility events.
Comment 9 Andrew Morris 2017-08-16 00:01:16 PDT
(In reply to youenn fablet from comment #8)
> Before doing that, can you do a quick test: try to restart playing the black
> video by calling play() when coming back from Siri.
> It might be that we are not restarting properly the autoplayed video that
> got paused due to backgrounding.
> 
> In that case, a temporary workaround would be to catch page visibility
> events.

Gave this a go. .play() claims to succeed, but does not actually play the video or result in any visual change.
Comment 10 Lucas Forschler 2017-08-16 08:55:27 PDT
The content of attachment 318209 [details] has been deleted for the following reason:

user request
Comment 11 youenn fablet 2017-08-16 14:04:48 PDT
Just tried and backgrounding does not make it.
Using Siri, the audio stops playing and when coming back to the page, the video is not always playing either.

Doing a video.play seems to trigger back the audio in the tests I did.
Same for audio, audio.play seems to relaunch it.
Comment 12 Andrew Morris 2017-08-16 17:17:39 PDT
Are you on a different build? Because it's 100% reproducible for me. On betas 4, 5, and 6 too, on different devices. I'm uploading a screen recording for you to compare, maybe there's something about what I'm doing that I didn't think to specify.
Comment 13 Andrew Morris 2017-08-16 17:19:08 PDT
Created attachment 318310 [details]
screen-recording-of-repro
Comment 14 youenn fablet 2017-08-16 18:21:29 PDT
Hum, I was able to reproduce it once and video.play() is not working also.
That is different from what I saw this morning with Siri interruption...

Clicking twice on the camera icon at the top-left is making the video going back though. What does the web site in this case?
Comment 15 Andrew Morris 2017-08-16 18:27:29 PDT
Created attachment 318314 [details]
switch-camera-icon
Comment 16 Andrew Morris 2017-08-16 18:29:16 PDT
@youenn: Do you mean this camera icon?
https://bugs.webkit.org/attachment.cgi?id=318314

That one is supposed to switch the local video between the available cameras.

Regarding Siri, that's an additional issue that occurs after getting the frozen remote video. Separate ticket for that here:
https://bugs.webkit.org/show_bug.cgi?id=175473
Comment 17 youenn fablet 2017-08-16 18:34:17 PDT
The camera icon like https://d30y9cdsu7xlg0.cloudfront.net/png/1998-200.png on the top left corner of the remote video
Comment 18 Andrew Morris 2017-08-16 18:42:00 PDT
Created attachment 318316 [details]
video-enabled-icon
Comment 19 Andrew Morris 2017-08-16 18:45:12 PDT
@youenn: Oh actually, I know the one you mean, it only shows over the video if you have focus. This one right?

https://bugs.webkit.org/attachment.cgi?id=318316

Anyway, I think I know what is happening. When switching to the background, decoding stops. That means when you resume, a keyframe is needed. Disabling and enabling the video using the icon above triggers a keyframe and unfreezes the video. If a new participant joins, a keyframe is generated for the new user, and incidentally is sent to everyone, which also unfreezes the video.

I think Safari should send a keyframe request when it resumes decoding, and this will fix it. Wdyt?
Comment 20 youenn fablet 2017-08-16 19:20:55 PDT
(In reply to Andrew Morris from comment #19)
> @youenn: Oh actually, I know the one you mean, it only shows over the video
> if you have focus. This one right?
> 
> https://bugs.webkit.org/attachment.cgi?id=318316
> 
> Anyway, I think I know what is happening. When switching to the background,
> decoding stops. That means when you resume, a keyframe is needed. Disabling
> and enabling the video using the icon above triggers a keyframe and
> unfreezes the video. If a new participant joins, a keyframe is generated for
> the new user, and incidentally is sent to everyone, which also unfreezes the
> video.

This is my hypothesis too.
But, then video should be coming back after 1 minute too, no?

> I think Safari should send a keyframe request when it resumes decoding, and
> this will fix it. Wdyt?

Yes, I think we need to fix that.
Comment 21 Andrew Morris 2017-08-16 19:32:56 PDT
(In reply to youenn fablet from comment #20)
> But, then video should be coming back after 1 minute too, no?

Yeah, I don't understand that either.

Also, I found a way to recover from the black videos when activating Siri - video.srcObject = video.srcObject. Seems to work reliably.
Comment 22 Andrew Morris 2017-08-16 19:36:17 PDT
Could be something about a different code path when a requested keyframe comes through vs when a regular keyframe comes through?

I have actually seen it unfreeze spontaneously a few times. But I've also tried to reproduce this by waiting 1min+ and failed.
Comment 23 youenn fablet 2017-09-21 11:21:36 PDT
Just tried to reproduce this case and was not able to do so with safari.opentokrtc.com.
Andrew, did anything changed there?
Comment 24 youenn fablet 2017-09-21 13:13:33 PDT
Created attachment 321469 [details]
Patch
Comment 25 Build Bot 2017-09-21 13:15:30 PDT
Attachment 321469 [details] did not pass style-queue:


ERROR: Source/ThirdParty/libwebrtc/Source/webrtc/sdk/objc/Framework/Classes/VideoToolbox/decoder.h:46:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/ThirdParty/libwebrtc/Source/webrtc/sdk/objc/Framework/Classes/VideoToolbox/decoder.h:46:  is_active is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/ThirdParty/libwebrtc/Source/webrtc/sdk/objc/Framework/Classes/VideoToolbox/decoder.h:57:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/ThirdParty/libwebrtc/Source/webrtc/sdk/objc/Framework/Classes/VideoToolbox/decoder.mm:102:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/ThirdParty/libwebrtc/Source/webrtc/sdk/objc/Framework/Classes/VideoToolbox/encoder.h:72:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/ThirdParty/libwebrtc/Source/webrtc/sdk/objc/Framework/Classes/VideoToolbox/encoder.h:72:  is_active is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/ThirdParty/libwebrtc/Source/webrtc/sdk/objc/Framework/Classes/VideoToolbox/encoder.h:99:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 7 in 20 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 26 youenn fablet 2017-09-21 16:10:31 PDT
Created attachment 321488 [details]
Patch
Comment 27 Build Bot 2017-09-21 16:13:50 PDT
Attachment 321488 [details] did not pass style-queue:


ERROR: Source/ThirdParty/libwebrtc/Source/webrtc/sdk/objc/Framework/Classes/VideoToolbox/encoder.h:72:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/ThirdParty/libwebrtc/Source/webrtc/sdk/objc/Framework/Classes/VideoToolbox/encoder.h:72:  is_active is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/ThirdParty/libwebrtc/Source/webrtc/sdk/objc/Framework/Classes/VideoToolbox/encoder.h:99:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/ThirdParty/libwebrtc/Source/webrtc/sdk/objc/Framework/Classes/VideoToolbox/decoder.mm:102:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/ThirdParty/libwebrtc/Source/webrtc/sdk/objc/Framework/Classes/VideoToolbox/decoder.h:46:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/ThirdParty/libwebrtc/Source/webrtc/sdk/objc/Framework/Classes/VideoToolbox/decoder.h:46:  is_active is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/ThirdParty/libwebrtc/Source/webrtc/sdk/objc/Framework/Classes/VideoToolbox/decoder.h:57:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 7 in 22 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 28 Andrew Morris 2017-09-21 17:39:00 PDT
@youenn Yep we have a workaround deployed where it disables & re-enables the video on Safari when we get visibility back.
Comment 29 youenn fablet 2017-09-22 08:34:34 PDT
Created attachment 321549 [details]
Patch
Comment 30 Build Bot 2017-09-22 10:36:32 PDT
Attachment 321549 [details] did not pass style-queue:


ERROR: Source/ThirdParty/libwebrtc/Source/webrtc/sdk/objc/Framework/Classes/VideoToolbox/encoder.h:72:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/ThirdParty/libwebrtc/Source/webrtc/sdk/objc/Framework/Classes/VideoToolbox/encoder.h:72:  is_active is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/ThirdParty/libwebrtc/Source/webrtc/sdk/objc/Framework/Classes/VideoToolbox/encoder.h:99:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/ThirdParty/libwebrtc/Source/webrtc/sdk/objc/Framework/Classes/VideoToolbox/decoder.mm:102:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/ThirdParty/libwebrtc/Source/webrtc/sdk/objc/Framework/Classes/VideoToolbox/decoder.h:46:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/ThirdParty/libwebrtc/Source/webrtc/sdk/objc/Framework/Classes/VideoToolbox/decoder.h:46:  is_active is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/ThirdParty/libwebrtc/Source/webrtc/sdk/objc/Framework/Classes/VideoToolbox/decoder.h:57:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 7 in 22 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 31 youenn fablet 2017-09-22 18:22:00 PDT
Created attachment 321607 [details]
Patch
Comment 32 Build Bot 2017-09-22 18:24:36 PDT
Attachment 321607 [details] did not pass style-queue:


ERROR: Source/ThirdParty/libwebrtc/Source/webrtc/sdk/objc/Framework/Classes/VideoToolbox/decoder.h:46:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/ThirdParty/libwebrtc/Source/webrtc/sdk/objc/Framework/Classes/VideoToolbox/decoder.h:46:  is_active is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/ThirdParty/libwebrtc/Source/webrtc/sdk/objc/Framework/Classes/VideoToolbox/decoder.h:57:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/ThirdParty/libwebrtc/Source/webrtc/sdk/objc/Framework/Classes/VideoToolbox/decoder.mm:102:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/ThirdParty/libwebrtc/Source/webrtc/sdk/objc/Framework/Classes/VideoToolbox/encoder.h:72:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/ThirdParty/libwebrtc/Source/webrtc/sdk/objc/Framework/Classes/VideoToolbox/encoder.h:72:  is_active is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/ThirdParty/libwebrtc/Source/webrtc/sdk/objc/Framework/Classes/VideoToolbox/encoder.h:99:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 7 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 33 Darin Adler 2017-09-23 17:29:26 PDT
Comment on attachment 321607 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=321607&action=review

Can’t tell if the SetActive that you are adding to libwebrtc is only for WEBRTC_WEBKIT_BUILD or if it’s supposed to be unconditionally added.

> Source/WebCore/platform/mediastream/libwebrtc/LibWebRTCProvider.cpp:118
> +    ASSERT(factoryAndThreads.audioDeviceModule);

This assertion seems a little silly. The line above assigns the result of make_unique to this. It’s not ambiguous.

> Source/WebCore/platform/mediastream/libwebrtc/LibWebRTCProvider.cpp:134
> +    auto* decoderFactory = new VideoToolboxVideoDecoderFactory();
> +    auto* encoderFactory = new VideoToolboxVideoEncoderFactory();

No need for the () here.

Strange to put these into raw pointers like this. I would have used unique_ptr. Maybe this is a single global object and we just let these leak?

> Source/WebCore/platform/mediastream/libwebrtc/LibWebRTCProvider.cpp:147
> -    if (!staticFactoryAndThreads().factory)
> +    if (!staticFactoryAndThreads().audioDeviceModule)
>          initializePeerConnectionFactoryAndThreads();

Code less logical now than it was before. Wish this could be written in a more straightforward way. Doesn’t seem like checking one specific field is a good way to guard this call. I think we should consider just making staticFactoryAndThreads() take care of initialization itself instead of doing it explicitly.

> Source/WebCore/platform/mediastream/libwebrtc/LibWebRTCProvider.cpp:160
> -    if (!factoryAndThreads.factory) {
> +    if (!factoryAndThreads.audioDeviceModule) {
>          staticFactoryAndThreads().networkThreadWithSocketServer = true;
>          initializePeerConnectionFactoryAndThreads();
>      }
>      ASSERT(staticFactoryAndThreads().networkThreadWithSocketServer);

This construction is a bit inelegant and strange. Why go out of our way to set networkThreadWithSocketServer only once? That creates a failure mode if someone calls setPeerConnectionFactory first and then calls createPeerConnection. I would like to see the assertions written in a more straightforward manner. They should focus on the actual risk, the actual bad way to call the functions. Is there some problem with a particular combination of calls, or could we just remove the assertion?

> Source/WebCore/platform/mediastream/libwebrtc/LibWebRTCProvider.h:83
> +    rtc::scoped_refptr<webrtc::PeerConnectionFactoryInterface> m_factory;
> +    VideoToolboxVideoDecoderFactory* m_decoderFactory  { nullptr };
> +    VideoToolboxVideoEncoderFactory* m_encoderFactory { nullptr };

Seems strange to use a RefPtr for the factory, but then raw pointers for the decoder and encoder factories. I suggest considering unique_ptr instead or raw pointers for all maybe?

Extra space after m_decoderFactory here.

> Source/WebCore/platform/mediastream/libwebrtc/VideoToolBoxDecoderFactory.cpp:39
> +    for (webrtc::H264VideoToolboxDecoder& decoder : m_decoders)

Too bad reference_wrapper forces you to write out the type here to avoid having to write () or get(). I would probably have used auto& and written the () out.

> Source/WebCore/platform/mediastream/libwebrtc/VideoToolBoxDecoderFactory.cpp:47
> +    auto* decoder = webrtc::VideoToolboxVideoDecoderFactory::CreateVideoDecoder(type);
> +    if (decoder)
> +        m_decoders.append(static_cast<webrtc::H264VideoToolboxDecoder&>(*decoder));

What makes this cast safe? Seems like a design mistake that it returns a generic type by somehow we know the exact class. Is there something we can do to correct this?

> Source/WebCore/platform/mediastream/libwebrtc/VideoToolBoxDecoderFactory.cpp:56
> +    m_decoders.removeFirstMatching([&] (const auto& item) {
> +        return &item.get() == decoder;
> +    });

Why use a Vector if we need the remove operation? I suggest a HashSet instead.

> Source/WebCore/platform/mediastream/libwebrtc/VideoToolBoxEncoderFactory.cpp:41
> +    for (H264VideoToolboxEncoder& encoder : m_encoders)

Ditto.

> Source/WebCore/platform/mediastream/libwebrtc/VideoToolBoxEncoderFactory.h:48
> +    Vector<std::reference_wrapper<H264VideoToolboxEncoder>> m_encoders;

Ditto.
Comment 34 youenn fablet 2017-09-25 11:53:28 PDT
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=321607&action=review
> 
> Can’t tell if the SetActive that you are adding to libwebrtc is only for
> WEBRTC_WEBKIT_BUILD or if it’s supposed to be unconditionally added.

Ideally, libwebrtc should not require its decoder/encoder to have a UIKit dependency and SetActive would be used for all.
For now it is indeed better to keep it to WEBRTC_WEBKIT_BUILD only.

> > Source/WebCore/platform/mediastream/libwebrtc/LibWebRTCProvider.cpp:118
> > +    ASSERT(factoryAndThreads.audioDeviceModule);
> 
> This assertion seems a little silly. The line above assigns the result of
> make_unique to this. It’s not ambiguous.

I'll remove it.


> > Source/WebCore/platform/mediastream/libwebrtc/LibWebRTCProvider.cpp:134
> > +    auto* decoderFactory = new VideoToolboxVideoDecoderFactory();
> > +    auto* encoderFactory = new VideoToolboxVideoEncoderFactory();
> 
> No need for the () here.
OK

> Strange to put these into raw pointers like this. I would have used
> unique_ptr. Maybe this is a single global object and we just let these leak?

I will use unique_ptr and release them when passing them to the factory
That will clarify how the API is supposed to be used.
Ideally, the factory API would take unique_tr&& as input.

> > Source/WebCore/platform/mediastream/libwebrtc/LibWebRTCProvider.cpp:147
> > -    if (!staticFactoryAndThreads().factory)
> > +    if (!staticFactoryAndThreads().audioDeviceModule)
> >          initializePeerConnectionFactoryAndThreads();
> 
> Code less logical now than it was before. Wish this could be written in a
> more straightforward way. Doesn’t seem like checking one specific field is a
> good way to guard this call. I think we should consider just making
> staticFactoryAndThreads() take care of initialization itself instead of
> doing it explicitly.


I'll try to refactor this.

> > Source/WebCore/platform/mediastream/libwebrtc/LibWebRTCProvider.cpp:160
> > -    if (!factoryAndThreads.factory) {
> > +    if (!factoryAndThreads.audioDeviceModule) {
> >          staticFactoryAndThreads().networkThreadWithSocketServer = true;
> >          initializePeerConnectionFactoryAndThreads();
> >      }
> >      ASSERT(staticFactoryAndThreads().networkThreadWithSocketServer);
> 
> This construction is a bit inelegant and strange. Why go out of our way to
> set networkThreadWithSocketServer only once? That creates a failure mode if
> someone calls setPeerConnectionFactory first and then calls
> createPeerConnection. I would like to see the assertions written in a more
> straightforward manner. They should focus on the actual risk, the actual bad
> way to call the functions. Is there some problem with a particular
> combination of calls, or could we just remove the assertion?

We should really make networkThreadWithSocketServer a process global variable.
This would be best as a follow_up.

> > Source/WebCore/platform/mediastream/libwebrtc/LibWebRTCProvider.h:83
> > +    rtc::scoped_refptr<webrtc::PeerConnectionFactoryInterface> m_factory;
> > +    VideoToolboxVideoDecoderFactory* m_decoderFactory  { nullptr };
> > +    VideoToolboxVideoEncoderFactory* m_encoderFactory { nullptr };
> 
> Seems strange to use a RefPtr for the factory, but then raw pointers for the
> decoder and encoder factories. I suggest considering unique_ptr instead or
> raw pointers for all maybe?

The factory is keeping the raw pointers alive.

> Extra space after m_decoderFactory here.
> 
> > Source/WebCore/platform/mediastream/libwebrtc/VideoToolBoxDecoderFactory.cpp:39
> > +    for (webrtc::H264VideoToolboxDecoder& decoder : m_decoders)
> 
> Too bad reference_wrapper forces you to write out the type here to avoid
> having to write () or get(). I would probably have used auto& and written
> the () out.
> 
> > Source/WebCore/platform/mediastream/libwebrtc/VideoToolBoxDecoderFactory.cpp:47
> > +    auto* decoder = webrtc::VideoToolboxVideoDecoderFactory::CreateVideoDecoder(type);
> > +    if (decoder)
> > +        m_decoders.append(static_cast<webrtc::H264VideoToolboxDecoder&>(*decoder));
> 
> What makes this cast safe? Seems like a design mistake that it returns a
> generic type by somehow we know the exact class. Is there something we can
> do to correct this?

To fix that, we would need to be more invasive into libwebrtc code by adding a type-specific method to VideoToolBoxDecoderFactory.

> > Source/WebCore/platform/mediastream/libwebrtc/VideoToolBoxDecoderFactory.cpp:56
> > +    m_decoders.removeFirstMatching([&] (const auto& item) {
> > +        return &item.get() == decoder;
> > +    });
> 
> Why use a Vector if we need the remove operation? I suggest a HashSet
> instead.

OK
Comment 35 youenn fablet 2017-09-25 14:25:30 PDT
Created attachment 321740 [details]
Patch
Comment 36 youenn fablet 2017-09-25 14:26:15 PDT
Created attachment 321742 [details]
Patch
Comment 37 Build Bot 2017-09-25 14:29:27 PDT
Attachment 321742 [details] did not pass style-queue:


ERROR: Source/ThirdParty/libwebrtc/Source/webrtc/sdk/objc/Framework/Classes/VideoToolbox/encoder.h:71:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/ThirdParty/libwebrtc/Source/webrtc/sdk/objc/Framework/Classes/VideoToolbox/encoder.h:71:  is_active is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/ThirdParty/libwebrtc/Source/webrtc/sdk/objc/Framework/Classes/VideoToolbox/encoder.h:98:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/ThirdParty/libwebrtc/Source/webrtc/sdk/objc/Framework/Classes/VideoToolbox/decoder.mm:102:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/ThirdParty/libwebrtc/Source/webrtc/sdk/objc/Framework/Classes/VideoToolbox/decoder.h:46:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/ThirdParty/libwebrtc/Source/webrtc/sdk/objc/Framework/Classes/VideoToolbox/decoder.h:46:  is_active is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/ThirdParty/libwebrtc/Source/webrtc/sdk/objc/Framework/Classes/VideoToolbox/decoder.h:57:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 7 in 22 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 38 youenn fablet 2017-09-25 14:32:41 PDT
I'll do the Vector -> HashSet as a follow-up, it requires adding a DefaultHash for reference_wrapper
Comment 39 WebKit Commit Bot 2017-09-25 15:33:34 PDT
Comment on attachment 321742 [details]
Patch

Rejecting attachment 321742 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 321742, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
:040000 040000 093903938a20571e7b7d4f513da310b393f68a03 172a1449945fc84a61d1db015da30b56a29cc36c M	WebKit.xcworkspace
Current branch master is up to date.
ERROR: Not all changes have been committed into SVN, however the committed
ones (if any) seem to be successfully integrated into the working tree.
Please see the above messages for details.


Failed to run "['git', 'svn', 'dcommit', '--rmdir']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit
Updating OpenSource
Current branch master is up to date.

Full output: http://webkit-queues.webkit.org/results/4654987
Comment 40 WebKit Commit Bot 2017-09-25 16:23:31 PDT
Comment on attachment 321742 [details]
Patch

Clearing flags on attachment: 321742

Committed r222478: <http://trac.webkit.org/changeset/222478>
Comment 41 WebKit Commit Bot 2017-09-25 16:23:34 PDT
All reviewed patches have been landed.  Closing bug.
Comment 42 Jon Lee 2017-12-13 10:49:24 PST
*** Bug 175473 has been marked as a duplicate of this bug. ***