Open Bug 1374420 Opened 7 years ago Updated 2 years ago

Streamline the PCameras capture capability API to reduce IPC calls

Categories

(Core :: WebRTC, enhancement, P3)

Product:
Core
Component:
WebRTC
Type:
enhancement

Tracking

()

People

(Reporter: ng, Assigned: ng)

Details

Attachments

(2 files, 1 obsolete file)

Bug 1374420 - streamline capture capabilities IPC API;
59 bytes, text/x-review-board-request
Details
Bug 1374420 - streamline capture capabilities IPC API;
59 bytes, text/x-review-board-request
jib
: review+
Details 
Enumerating capture capabilities is done one-at-a-time over IPC today. This can result in 10s to 100s of IPC calls. It can be simplified to return the entire list of capabilities in a single call.
Rank: 25
Comment on attachment 8879390 [details]
Bug 1374420 - streamline capture capabilities IPC API;

https://reviewboard.mozilla.org/r/150710/#review155610

Thanks for doing this! Looks good, some nits and ideas.

While I don't see much utility for the IpcToWebrtcCapability/WebrtcToIpcCapability outside of CamerasParent/Child.cpp, any other place we discussed moving them to seems more public, so perhaps they're fine where they are? Either way.

I've endulged in some ideas for further cleanup inline. Feel free to ignore. r+ really, but I'd like to see patch again for nits, so unless I set r- bugzilla does a poor job of notifying me. :/

::: dom/media/systemservices/CamerasChild.h:196
(Diff revision 1)
> +  GetCaptureCapabilities(CaptureEngine aCapEngine,
> -                           const char* unique_idUTF8,
> +                       const char* unique_idUTF8,
> -                           const unsigned int capability_number,
> +                       nsTArray<webrtc::VideoCaptureCapability>& capabilities);

Nit: align args with first arg.

::: dom/media/systemservices/CamerasChild.cpp:306
(Diff revision 1)
> -CamerasChild::GetCaptureCapability(CaptureEngine aCapEngine,
> +CamerasChild::GetCaptureCapabilities(CaptureEngine aCapEngine,
>                                     const char* unique_idUTF8,
> -                                   const unsigned int capability_number,
> -                                   webrtc::VideoCaptureCapability& capability)
> +                                   nsTArray<webrtc::VideoCaptureCapability>&
> +                                       capabilityList)

Nit: align secondary args with first arg or indent them by 4 to avoid wordwrapping individual args.

::: dom/media/systemservices/CamerasChild.cpp:334
(Diff revision 1)
>    MonitorAutoLock monitor(mReplyMonitor);
>    mReceivedReply = true;
>    mReplySuccess = true;
> -  mReplyCapability.width = ipcCapability.width();
> -  mReplyCapability.height = ipcCapability.height();
> -  mReplyCapability.maxFPS = ipcCapability.maxFPS();
> +  mReplyCapabilities.Clear();
> +  webrtc::VideoCaptureCapability capability;
> +  for(auto ipcCapability : ipcCapabilityList) {

auto&

We don't want to be copying these. Also space after for

::: dom/media/systemservices/CamerasParent.cpp:487
(Diff revision 1)
> -CamerasParent::RecvNumberOfCapabilities(const CaptureEngine& aCapEngine,
> +CamerasParent::RecvGetCaptureCapabilities(const CaptureEngine& aCapEngine,
>                                          const nsCString& unique_id)
>  {
> -  LOG((__PRETTY_FUNCTION__));
> -  LOG(("Getting caps for %s", unique_id.get()));
> +  LOG(("%s RecvGetCaptureCapability: %s", __PRETTY_FUNCTION__,
> +       unique_id.get()));

Nit: redundant-looking log comment (\_\_PRETTY_FUNCTION\_\_ == "RecvGetCaptureCapabilities")

::: dom/media/systemservices/CamerasParent.cpp:497
(Diff revision 1)
> -  LOG(("RecvGetCaptureCapability: %s %d", unique_id.get(), num));
> -
> -  RefPtr<CamerasParent> self(this);
> -  RefPtr<Runnable> webrtc_runnable =
> -    media::NewRunnableFrom([self, unique_id, aCapEngine, num]() -> nsresult {
>        webrtc::VideoCaptureCapability webrtcCaps;

Can be moved closer to use (right outside for-loop)

::: dom/media/systemservices/CamerasParent.cpp:502
(Diff revision 1)
> +          auto num = devInfo->NumberOfCapabilities(unique_id.get());
> +          for (auto i = 0; i < num; i++) {

I'd prefer int32_t to auto on these two, especially the second one!

::: dom/media/systemservices/VideoCaptureUtils.cpp:23
(Diff revision 1)
> +}
> +
> +void VideoCaptureUtils::WebrtcToIpcCapability(
> +    const webrtc::VideoCaptureCapability& webrtc,
> +    VideoCaptureCapability& ipc) {
> +  ipc.width()= webrtc.width;

space

::: dom/media/webrtc/MediaEngineCameraVideoSource.h:67
(Diff revision 1)
> -    explicit CapabilityCandidate(uint8_t index, uint32_t distance = 0)
> -    : mIndex(index), mDistance(distance) {}
> -
> +    explicit CapabilityCandidate(uint8_t index,
> +                                const webrtc::CaptureCapability cap,
> +                                uint32_t distance = 0)

Nit: align args

::: dom/media/webrtc/MediaEngineCameraVideoSource.h:67
(Diff revision 1)
> -    explicit CapabilityCandidate(uint8_t index, uint32_t distance = 0)
> -    : mIndex(index), mDistance(distance) {}
> -
> +    explicit CapabilityCandidate(uint8_t index,
> +                                const webrtc::CaptureCapability cap,
> +                                uint32_t distance = 0)
> +    : mIndex(index), mCapability(cap), mDistance(distance) {}

Remove unused index and mIndex.

::: dom/media/webrtc/MediaEngineCameraVideoSource.h:72
(Diff revision 1)
> -    explicit CapabilityCandidate(uint8_t index, uint32_t distance = 0)
> -    : mIndex(index), mDistance(distance) {}
> -
> +    explicit CapabilityCandidate(uint8_t index,
> +                                const webrtc::CaptureCapability cap,
> +                                uint32_t distance = 0)
> +    : mIndex(index), mCapability(cap), mDistance(distance) {}
>      size_t mIndex;
> +    const webrtc::CaptureCapability mCapability;

It's tempting to use

    const webrtc::CaptureCapability& mCapability;

here since I believe uses of CapabilityCandidate are stack-only.

We could mark CapabilityCandidate with MOZ_STACK_CLASS to help enforce this.

Thoughts?

::: dom/media/webrtc/MediaEngineCameraVideoSource.cpp:46
(Diff revision 1)
> -  aOut = mHardcodedCapabilities.SafeElementAt(aIndex, webrtc::CaptureCapability());
> +  for(size_t i = 0; i < mHardcodedCapabilities.Length(); ++i) {
> +    webrtc::CaptureCapability cap;
> +    cap = mHardcodedCapabilities.SafeElementAt(i, webrtc::CaptureCapability());
> +    aOut.AppendElement(CapabilityCandidate(i, cap));

We can use for( : ) here now that the CapabilityCandidate() constructor no longer needs an index.

::: dom/media/webrtc/MediaEngineCameraVideoSource.cpp:255
(Diff revision 1)
> -      webrtc::CaptureCapability cap;
> -      GetCapability(candidate.mIndex, cap);
> +      if (GetFitnessDistance(candidate.mCapability, cs, aDeviceId)
> +              == UINT32_MAX) {

Nit: One line?

::: dom/media/webrtc/MediaEngineRemoteVideoSource.cpp:476
(Diff revision 1)
>    if (!mHardcodedCapabilities.IsEmpty()) {
> -    MediaEngineCameraVideoSource::GetCapability(aIndex, aOut);
> -  }
> +    MediaEngineCameraVideoSource::GetCapabilityCandidateSet(aOut);
> +  } else {

I think we should remove the test on mHardcodedCapabilities.IsEmpty() here (actually move it down with aOut.IsEmpty() below so we don't add mHardcodedCapabilities if they already exist).

This matches the old code better where we called CamerasChild::GetCaptureCapability() regardless of mHardcodedCapabilities.IsEmpty(), overwriting the entry (last wins).

At one point we had doubts about whether CamerasChild::GetCaptureCapabilities() could ever return zero initially, and change its mind later. Maybe it can't but this seems safer, so we never block capabilities if they exist.

::: dom/media/webrtc/MediaEngineWebRTC.cpp:223
(Diff revision 1)
> -    webrtc::CaptureCapability cap;
> -    int numCaps = mozilla::camera::GetChildAndCall(
> -      &mozilla::camera::CamerasChild::NumberOfCapabilities,
> -      capEngine,
> -      uniqueId);
> -    LOG(("Number of Capabilities %d", numCaps));
> +    if (!mozilla::camera::GetChildAndCall(
> +          &mozilla::camera::CamerasChild::GetCaptureCapabilities,
> +          capEngine, uniqueId, capList))
> +    {
> +      LOG(("Number of Capabilities %" PRIuSIZE, capList.Length()));
> +      for (auto cap : capList) {

auto&
Attachment #8879390 - Flags: review?(jib) → review-
Comment on attachment 8879390 [details]
Bug 1374420 - streamline capture capabilities IPC API;

https://reviewboard.mozilla.org/r/150710/#review155610

> I'd prefer int32_t to auto on these two, especially the second one!

Good catch.

> It's tempting to use
> 
>     const webrtc::CaptureCapability& mCapability;
> 
> here since I believe uses of CapabilityCandidate are stack-only.
> 
> We could mark CapabilityCandidate with MOZ_STACK_CLASS to help enforce this.
> 
> Thoughts?

It would be nice not to have to make a copy, but I don't think we can take a ref for mCapability because it is allocated at a deeper stack level than it is used.

> Nit: One line?

Sadly, two.
Comment on attachment 8879390 [details]
Bug 1374420 - streamline capture capabilities IPC API;

https://reviewboard.mozilla.org/r/150710/#review155610

I can't add a virtual destructor to the webrtc struct, so I have to pass on the polymorphic converter idea, shame.
Comment on attachment 8879390 [details]
Bug 1374420 - streamline capture capabilities IPC API;

https://reviewboard.mozilla.org/r/150710/#review155610

Oh I was just thinking it'd be non-virtual with a copy constructor. Maybe later.

> It would be nice not to have to make a copy, but I don't think we can take a ref for mCapability because it is allocated at a deeper stack level than it is used.

Maybe we can change it to a constructor... I'll ponder it for another time.
Comment on attachment 8879390 [details]
Bug 1374420 - streamline capture capabilities IPC API;

https://reviewboard.mozilla.org/r/150710/#review156300

lgtm with nits.

::: dom/media/systemservices/CamerasChild.cpp:334
(Diff revisions 1 - 2)
>    MonitorAutoLock monitor(mReplyMonitor);
>    mReceivedReply = true;
>    mReplySuccess = true;
>    mReplyCapabilities.Clear();
>    webrtc::VideoCaptureCapability capability;
> -  for(auto ipcCapability : ipcCapabilityList) {
> +  for(auto& ipcCapability : ipcCapabilityList) {

for space (

::: dom/media/webrtc/MediaEngineCameraVideoSource.cpp:46
(Diff revisions 1 - 2)
>  
>  void
>  MediaEngineCameraVideoSource::GetCapabilityCandidateSet(CapabilitySet& aOut) const
>  {
>    aOut.Clear();
> -  for(size_t i = 0; i < mHardcodedCapabilities.Length(); ++i) {
> +  for (auto& cap: mHardcodedCapabilities) {

cap space :

::: dom/media/webrtc/MediaEngineRemoteVideoSource.cpp:483
(Diff revisions 1 - 2)
>      mozilla::camera::GetChildAndCall(
>        &mozilla::camera::CamerasChild::GetCaptureCapabilities,
>        mCapEngine,
>        GetUUID().get(),
>        caps);
> -    size_t i = 0;
> +    for (auto& cap: caps) {

cap space :

::: dom/media/webrtc/MediaEngineRemoteVideoSource.cpp:487
(Diff revisions 1 - 2)
> -    mHardcodedCapabilities.Clear();
> +    if (!mHardcodedCapabilities.IsEmpty()) {
> +      MediaEngineCameraVideoSource::GetCapabilityCandidateSet(aOut);
> +      return;
> +    }
>      // The default for devices that don't return discrete capabilities: treat
>      // them as supporting all capabilities orthogonally. E.g. screensharing.
>      // CaptureCapability defaults key values to 0, which means accept any value.
>      mHardcodedCapabilities.AppendElement(webrtc::CaptureCapability());
>      // Return the hardcoded set of capabilities (now of size 1)
>      MediaEngineCameraVideoSource::GetCapabilityCandidateSet(aOut);
>    }
>  }

This is a rare case where early-bail is less desirable imho (saves duplicating one line).
Attachment #8879390 - Flags: review?(jib) → review+
Comment on attachment 8879390 [details]
Bug 1374420 - streamline capture capabilities IPC API;

https://reviewboard.mozilla.org/r/150710/#review156376
Attachment #8879390 - Attachment is obsolete: true
Attachment #8892631 - Flags: review?(jib)
Attachment #8892631 - Attachment is obsolete: true
Attachment #8892631 - Attachment is obsolete: false
Comment on attachment 8892631 [details]
Bug 1374420 - streamline capture capabilities IPC API;

https://reviewboard.mozilla.org/r/163616/#review168986
Comment on attachment 8892649 [details]
Bug 1374420 - streamline capture capabilities IPC API;

https://reviewboard.mozilla.org/r/163628/#review169500
Attachment #8892649 - Flags: review?(jib) → review+
Mass change P2->P3 to align with new Mozilla triage process.
Priority: P2 → P3
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: