[webkit-reviews] review granted: [Bug 175472] WebRTC video does not resume receiving when switching back to Safari 11 on iOS : [Attachment 321607] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted youenn fablet <youennf at gmail.com>'s
request for review:
Bug 175472: WebRTC video does not resume receiving when switching back to
Safari 11 on iOS
https://bugs.webkit.org/show_bug.cgi?id=175472

Attachment 321607: Patch

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




--- Comment #33 from Darin Adler <darin at apple.com> ---
Comment on attachment 321607
  --> https://bugs.webkit.org/attachment.cgi?id=321607
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.