1060249 - Disable WebRTC content analysis of video frames for motion/complexity on Gonk

Status: RESOLVED FIXED

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

Description

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

[Blocking Requested - why for this release]:

Disable the motion/spatial analysis on b2g (simply ifdef out the call), and hard-code the results to be "normal" for both (in a range of "low/normal/high").  (perhaps "high" for motion to create a preference for framerate over resolution).  This will let the qm_select code operate normally.

Inheriting the blocking request from bug 972639 (we'll want some verification it works and does in fact lower CPU use (or increase frame rate)).

Comment 1

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

Attachment: disable frame motion/complexity analysis in webrtc on Gonk

Comment 2

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8481128 [details] [diff] [review]
disable frame motion/complexity analysis in webrtc on Gonk

Review of attachment 8481128 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good but I don't see you *actually disabling* the analysis.

http://dxr.mozilla.org/mozilla-central/source/media/webrtc/trunk/webrtc/video_engine/vie_encoder.cc#210
http://dxr.mozilla.org/mozilla-central/source/media/webrtc/trunk/webrtc/video_engine/vie_encoder.cc#291

Also note this:

http://dxr.mozilla.org/mozilla-central/source/media/webrtc/trunk/webrtc/modules/video_processing/main/source/frame_preprocessor.cc#131

It's currently set to 2. Maybe we can just bump that up.

Comment 3

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

Attachment: disable frame motion/complexity analysis in webrtc on Gonk

Comment 4

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

Attachment: disable frame motion/complexity analysis in webrtc on Gonk

Comment 5

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

Comment on attachment 8481249 [details] [diff] [review]
disable frame motion/complexity analysis in webrtc on Gonk

Thanks - I also disabled allocation of the unneeded prev_frame_ buffer

Comment 6

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

(In reply to Gian-Carlo Pascutto [:gcp] from comment #2)
> Comment on attachment 8481128 [details] [diff] [review]
> disable frame motion/complexity analysis in webrtc on Gonk
> 
> Review of attachment 8481128 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good but I don't see you *actually disabling* the analysis.
> 
> http://dxr.mozilla.org/mozilla-central/source/media/webrtc/trunk/webrtc/
> video_engine/vie_encoder.cc#210
> http://dxr.mozilla.org/mozilla-central/source/media/webrtc/trunk/webrtc/
> video_engine/vie_encoder.cc#291
> 
> Also note this:
> 
> http://dxr.mozilla.org/mozilla-central/source/media/webrtc/trunk/webrtc/
> modules/video_processing/main/source/frame_preprocessor.cc#131
> 
> It's currently set to 2. Maybe we can just bump that up.

We could, but:  'motion' would be measured as unnaturally high, and it would still pay the memory and CPU cost for copying the frames (though at 1/Nth the cost).  That is a viable alternative - bump it from 2 to say 20 should cut the CPU use to <1% (very likely <0.5%).  Comments?  of course, that patch is effectively a 1-liner-ifdef

Comment 7

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8481249 [details] [diff] [review]
disable frame motion/complexity analysis in webrtc on Gonk

Review of attachment 8481249 [details] [diff] [review]:
-----------------------------------------------------------------

Looks OK. How do you feel about just nuking this:
http://dxr.mozilla.org/mozilla-central/source/media/webrtc/trunk/webrtc/modules/video_coding/main/source/media_optimization.cc#438
then in VCMContentMetricsProcessing::UpdateContentData handle the ContentMetrics == NULL case with the "average values" you added in this patch, and modifying the enableContentAnalysis calls I pointed out in the previous review?

I think that's slightly cleaner than this, but I can live with it, too.

Comment 8

[Gian-Carlo Pascutto [:gcp]]

(In reply to Randell Jesup [:jesup] from comment #6)

> We could, but:  'motion' would be measured as unnaturally high, and it would
> still pay the memory and CPU cost for copying the frames (though at 1/Nth
> the cost).  That is a viable alternative - bump it from 2 to say 20 should
> cut the CPU use to <1% (very likely <0.5%).  Comments?  of course, that
> patch is effectively a 1-liner-ifdef

I sortof agree that if we plan to get rid of this anyway, then keeping the analysis
enabled but in a way that we don't even know if it returns sensible results doesn't
do us much good.

Comment 9

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/13c8d9e68cb6

Comment 10

[Michael Vines [:m1] [:evilmachines]]

Jay, can you please confirm that this patch helps w/ CPU load to help turn the v2.0? into a +

Comment 11

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/2b60f30db362 for something in that push causing b2g build bustage like https://tbpl.mozilla.org/php/getParsedLog.php?id=47078117&tree=Mozilla-Inbound

Comment 12

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/d07a9cabc185
This was the cause of the bustage; I didn't remove a bit that was no longer needed

Comment 13

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

jay - use the final landed patch (probably with the patch from the other bug as well to pass contentMetrics to h.264 AddVideoFrame).  The gmp patch you don't need, and won't apply to 2.0.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d07a9cabc185

Comment 15

[Jay]

(In reply to Michael Vines [:m1] [:evilmachines] from comment #10)
> Jay, can you please confirm that this patch helps w/ CPU load to help turn
> the v2.0? into a +

Just check the patch https://hg.mozilla.org/mozilla-central/rev/d07a9cabc185 and https://hg.mozilla.org/mozilla-central/rev/f8055a6d7d10. The CPU utilization improved by 2.56%.

Comment 16

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

(In reply to Jay from comment #15)
> (In reply to Michael Vines [:m1] [:evilmachines] from comment #10)
> > Jay, can you please confirm that this patch helps w/ CPU load to help turn
> > the v2.0? into a +
> 
> Just check the patch https://hg.mozilla.org/mozilla-central/rev/d07a9cabc185
> and https://hg.mozilla.org/mozilla-central/rev/f8055a6d7d10. The CPU
> utilization improved by 2.56%.

Thanks; that seems to be what I would expect.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Note that due to recent policy changes, all B2G uplifts needs approval now regardless of blocking status. Please request b2g32 approval on this patch when you get a chance. Sorry for the inconvenience.

Comment 18

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

Comment on attachment 8481249 [details] [diff] [review]
disable frame motion/complexity analysis in webrtc on Gonk

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

(Missed setting the patch b2g32? flag when the other blockers were resolved)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): n/a

User impact if declined: Significant extra CPU (and some memory) use on B2G

Testing completed: On nightly and 34 for some time. Tested by myself and by QC; 2.5+% CPU reduction.

Risk to taking this patch (and alternatives if risky): Very low risk.  Avoids calling expensive code.

String or UUID changes made by this patch: none

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/2a7061aa9bff

Comment 20

[Kai-Zhen Li [:seinlin][:kli]]

http://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/2a7061aa9bff