Bug 205645 – Apply rotation at source level if WebRTC sink ask so

• Status: RESOLVED
• Resolution: FIXED
• Component: WebRTC

[youenn fablet]

Apply rotation at source level if WebRTC sink ask so.
This allows to do rotation in UIProcess/GPUProcess if needed.

[youenn fablet]

Created attachment 386530 [details]
Patch

[youenn fablet]

Created attachment 386538 [details]
Patch

[Eric Carlson]

Comment on attachment 386538 [details]
Patch

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

> Source/WebCore/platform/graphics/cv/ImageRotationSessionVT.mm:123
> +    IntSize size { (int)CVPixelBufferGetWidth(pixelBuffer) , (int)CVPixelBufferGetHeight(pixelBuffer) };

s/(pixelBuffer) , (int)/(pixelBuffer), (int)/

> Source/WebCore/platform/mediastream/RealtimeOutgoingVideoSource.cpp:94
> +    if (!shouldApplyRotation || shouldApplyRotation == m_areSinksAskingToApplyRotation)
> +        return;
> +
> +    m_areSinksAskingToApplyRotation = true;
> +    if (!m_videoSource->source().setShouldApplyRotation(true))
> +        m_shouldApplyRotation = true;

Don't you want to clear m_areSinksAskingToApplyRotation and unapply rotation to the source if passed shouldApplyRotation==false?

[youenn fablet]

Created attachment 386540 [details]
Patch

[youenn fablet]

> > +    m_areSinksAskingToApplyRotation = true;
> > +    if (!m_videoSource->source().setShouldApplyRotation(true))
> > +        m_shouldApplyRotation = true;
> 
> Don't you want to clear m_areSinksAskingToApplyRotation and unapply rotation
> to the source if passed shouldApplyRotation==false?

Not really, for instance if the same track is given to two peer connections one with CVO enabled and the other with CVO disabled.
It is simpler to enable rotation at source level and never disable it.
In practice, this should happen rarely.

[Eric Carlson]

(In reply to youenn fablet from comment #5)
> > > +    m_areSinksAskingToApplyRotation = true;
> > > +    if (!m_videoSource->source().setShouldApplyRotation(true))
> > > +        m_shouldApplyRotation = true;
> > 
> > Don't you want to clear m_areSinksAskingToApplyRotation and unapply rotation
> > to the source if passed shouldApplyRotation==false?
> 
> Not really, for instance if the same track is given to two peer connections
> one with CVO enabled and the other with CVO disabled.
> It is simpler to enable rotation at source level and never disable it.
> In practice, this should happen rarely.

Then why have the 'shouldApplyRotation' parameter at all?

[youenn fablet]

Created attachment 386551 [details]
Patch

[youenn fablet]

(In reply to Eric Carlson from comment #6)
> (In reply to youenn fablet from comment #5)
> > > > +    m_areSinksAskingToApplyRotation = true;
> > > > +    if (!m_videoSource->source().setShouldApplyRotation(true))
> > > > +        m_shouldApplyRotation = true;
> > > 
> > > Don't you want to clear m_areSinksAskingToApplyRotation and unapply rotation
> > > to the source if passed shouldApplyRotation==false?
> > 
> > Not really, for instance if the same track is given to two peer connections
> > one with CVO enabled and the other with CVO disabled.
> > It is simpler to enable rotation at source level and never disable it.
> > In practice, this should happen rarely.
> 
> Then why have the 'shouldApplyRotation' parameter at all?

True, I removed it from RealtimeOutgoingVideoSource.

[youenn fablet]

Created attachment 393143 [details]
Patch

[Eric Carlson]

Comment on attachment 393143 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        By default, the method does nothing and RealtimeOyutgoingVideoSource will continue to do the rotation itself.

s/RealtimeOyutgoingVideoSource/RealtimeOutgoingVideoSource/

> Source/WebCore/platform/graphics/cv/ImageRotationSessionVT.h:58
> +    ImageRotationSessionVT(const RotationProperties&, FloatSize, OSType pixelFormat, IsCGImageCompatible);

Nit: "pixelFormat" doesn't seem necessary.

> Source/WebCore/platform/graphics/cv/ImageRotationSessionVT.h:72
> +    void initialize(const RotationProperties&, FloatSize, OSType pixelFormat, IsCGImageCompatible);

Ditto.

> Source/WebCore/platform/graphics/cv/ImageRotationSessionVT.mm:101
>      CVPixelBufferPoolRef rawPool = nullptr;
>      if (auto err = CVPixelBufferPoolCreate(kCFAllocatorDefault, nullptr, (__bridge CFDictionaryRef)pixelAttributes, &rawPool); err != noErr)

I now this isn't new, but it seems like a bad idea to use a NULL buffer pool if this fails.

> Source/WebCore/platform/mediastream/RealtimeMediaSource.h:200
> +    virtual bool setShouldApplyRotation(bool /* shouldApplyRotation */) { return false; }

Nit: the method name is quite descriptive so the commented out parameter name doesn't seem necessary.

> Source/WebKit/UIProcess/Cocoa/UserMediaCaptureManagerProxy.cpp:195
> +    bool m_shouldApplyRotation { false };

It doesn't look like this is used.

> Source/WebKit/WebProcess/cocoa/UserMediaCaptureManager.cpp:226
> +    bool setShouldApplyRotation(bool /* shouldApplyRotation */) final;

Same comment as above.

[youenn fablet]

Created attachment 393464 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 393464 [details]
Patch

Clearing flags on attachment: 393464

Committed r258391: <https://trac.webkit.org/changeset/258391>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/60411815>

[Ryan Haddad]

Reverted in https://trac.webkit.org/changeset/258424/webkit because it broke internal builds. Build logs in radar.

[youenn fablet]

Created attachment 393644 [details]
Potential build fix

[youenn fablet]

Created attachment 393649 [details]
Potential build fix

[WebKit Commit Bot]

Comment on attachment 393649 [details]
Potential build fix

Clearing flags on attachment: 393649

Committed r258504: <https://trac.webkit.org/changeset/258504>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.