High CPU during video buffering in mp4_demuxer::Index::ConvertByteRangesToTimeRanges

RESOLVED FIXED

Status

()

P2
normal
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: mieischer, Assigned: jya)

Tracking

({perf, regression})

45 Branch
Unspecified
All
perf, regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox44 unaffected, firefox45+ verified)

Details

Attachments

(1 attachment)

(Reporter)

Description

3 years ago
Created attachment 8691012 [details]
high cpu usage - instruments.png

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.11; rv:45.0) Gecko/20100101 Firefox/45.0
Build ID: 20151123030237

Steps to reproduce:

1. Create new firefox profile (e10s/non-e10s doesn't matter)
2. Open a mp4 video that is several minutes long (for example http://peach.themazzone.com/durian/movies/sintel-1280-surround.mp4)
3. Pause the video and watch the CPU usage rise while it's loading


Actual results:

The CPU usage rises as more of the video is loaded. After about 5 minutes of the video have been loaded CPU usage peaks at approximately 130%.
This only happens while the video download is still in progress, once loading has finished the CPU usage is low again (may take some time to settle).
Once CPU usage has reached 130% it stays like this until the video is fully buffered. In addition during playback the video starts to stutter, that is it plays for a few seconds, gets stuck for a few seconds and so on.


Expected results:

Smooth video playback with little CPU usage (ca. 20% after buffering is finished).


Using Instruments shows that most of the CPU time is spent inside mp4_demuxer::Index::ConvertByteRangesToTimeRanges.
See the attached screenshot, which also shows the increasing cpu usage at the beginning.

Updated

3 years ago
Component: Untriaged → Audio/Video
Keywords: perf
Product: Firefox → Core

Comment 1

3 years ago
I can reproduce the high CPU usage.

Firefox38ESR  : 0-10%
Nightly45.0a1 : 0-30%(tested non-e10s)
(Core2Quad)
Status: UNCONFIRMED → NEW
Ever confirmed: true

Comment 2

3 years ago
[Tracking Requested - why for this release]: Video Stutterer and High CPU usage

Regression window:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=ada1afb12a16333d26974328d0f340712f354bf2&tochange=7fbfb74b3dd3

Regrssed by: 7fbfb74b3dd3	JW Wang — Bug 1223599 - Remove the throttling argument from AbstractMediaDecoder::NotifyDataArrived(). r=jya.
Blocks: 1223599
status-firefox44: --- → unaffected
status-firefox45: --- → affected
tracking-firefox45: --- → ?
Flags: needinfo?(jwwang)
OS: Unspecified → All
(Assignee)

Comment 3

3 years ago
may have been regressed by bug 1223599, as the reason we added the throttle parameters was precisely to reduce CPU usage with MP4.
See Also: → bug 1223599

Updated

3 years ago
Keywords: regression
Do you think we should restore the throttling mechanism? It looks like MP4TrackDemuxer::EnsureUpToDateIndex() is not optimized to skip processed buffer ranges when calling UpdateMoofIndex().
Flags: needinfo?(jyavenard)
(Assignee)

Comment 5

3 years ago
We could restore it for the time being, or localised the cache inside the MediaFormatReader so it doesn't call MP4Demuxer::NotifyDataArrived() too often.

Or best, modify the mp4_demuxer::Index to properly handle incremental update rather than doing like now of full reparse from 0 which include calling QuickSort.

It's not the first time the issue has arisen
Flags: needinfo?(jyavenard)
If mp4_demuxer::Index optimization can be done soon which waits for jya's call, I would prefer this approach.

Otherwise, I can throttle the calls to MP4Demuxer::NotifyDataArrived() in MediaFormatReader to alleviate the problem we have at hand. I can start working on this now.

Thoughts?
Flags: needinfo?(jwwang) → needinfo?(jyavenard)
(Assignee)

Comment 7

3 years ago
Gerald, is that something you could do? we've talked about it not long ago.
Flags: needinfo?(jyavenard) → needinfo?(gsquelart)
(Assignee)

Comment 8

3 years ago
One way to quickly remove the issue is have NotifyDataArrived queue another task rather than handling it directly.

The reason being is that NotifyDataArrived is called very quickly very often. If we were to re-queue the task, it only get processed once they have all arrived.

But I thought I had already done that
(Assignee)

Comment 9

3 years ago
Ah, that stuff is gone.

In MediaFormatReader::NotifyDataArrivedInternal() just requeue a task to NotifyDemuxer(); will immediately see the CPU usage drop to normal (it's a fix I had implemented in the past, seems to have been removed during refactoring)
(Assignee)

Updated

3 years ago
Depends on: 1227393
(Assignee)

Updated

3 years ago
Summary: High CPU during video buffering in mp4_demuxer::Index::ConvertByteRangesToTimeRanges → mp4_demuxer::Index::ConvertByteRangesToTimeRanges not handling incremental updates
(In reply to Jean-Yves Avenard [:jya] from comment #7)
> Gerald, is that something you could do? we've talked about it not long ago.
I've got plenty of bugs in my queue, and you'd be much quicker to do this, so I'd prefer to pass on this one, however interesting it seems! Happy to come back to it later on if it's still up for grab.
Flags: needinfo?(gsquelart)
Summary: mp4_demuxer::Index::ConvertByteRangesToTimeRanges not handling incremental updates → High CPU during video buffering in mp4_demuxer::Index::ConvertByteRangesToTimeRanges
(Assignee)

Updated

3 years ago
Depends on: 1227396
This bug should now be fixed thanks to bug 1227396.
Michael, would you please try again, see if the CPU usage is now acceptable on your machine?
Flags: needinfo?(mieischer)
Priority: -- → P2
(Reporter)

Comment 12

3 years ago
The CPU usage is really low now.
mozilla::MediaDecoderReader::GetBuffered() now amounts to less than 10% of the CPU usage of the plugin-container (which has less than 50 % CPU usage). mp4_demuxer::Index::ConvertByteRangesToTimeRanges itself only requires one third of the runtime of mozilla::MediaDecoderReader::GetBuffered().
Flags: needinfo?(mieischer)
Thank you Michael.

I'll mark this bug as fixed then.

(Assigning to :jya as he's done the actual work in bug 1227396.)
Assignee: nobody → jyavenard
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
status-firefox45: affected → verified
tracking-firefox45: ? → +
You need to log in before you can comment on or make changes to this bug.