Closed Bug 1228187 Opened 9 years ago Closed 8 years ago

Add a throttling argument to MediaDecoderReader::NotifyDataArrived()

Categories

(Core :: Audio/Video: Playback, defect, P2)

defect

Tracking

()

RESOLVED WONTFIX

People

(Reporter: jwwang, Assigned: jwwang)

References

Details

Attachments

(2 files)

Per bug 1227396 comment 21, we need to throttle calls to MediaDecoderReader::GetBuffered() which is heavy.
Assignee: nobody → jwwang
Depends on: 1227396
Depends on: 1223599
Bug 1228187. Part 1 - Add a throttling argument to MediaDecoderReader::NotifyDataArrived(). r=jya.
Attachment #8692679 - Flags: review?(jyavenard)
Bug 1228187. Part 2 - assert functions that shouldn't be called after Shutdown(). r=jya.
Attachment #8692680 - Flags: review?(jyavenard)
Comment on attachment 8692679 [details]
MozReview Request: Bug 1228187. Part 1 - Add a throttling argument to MediaDecoderReader::NotifyDataArrived(). r=jya.

https://reviewboard.mozilla.org/r/26305/#review23753

::: dom/media/MediaDecoderReader.cpp:207
(Diff revision 1)
> +MediaDecoderReader::NotifyDataArrived(bool aThrottleUpdates)

I would much prefer this to be handled in MediaDecoder like it used to be rather than in the reader.
Attachment #8692679 - Flags: review?(jyavenard)
Not sure what you mean by handling it in MediaDecoder. We used to have a function in MediaDecoderReader:

void DispatchNotifyDataArrived(bool aThrottleUpdates)
{
	RefPtr<nsRunnable> r = NS_NewRunnableMethod(
	  this,
	  aThrottleUpdates ? &MediaDecoderReader::ThrottledNotifyDataArrived :
	                     &MediaDecoderReader::NotifyDataArrived);

	OwnerThread()->Dispatch(
	  r.forget(), AbstractThread::DontAssertDispatchSuccess);
}
Attachment #8692679 - Flags: review+
Comment on attachment 8692679 [details]
MozReview Request: Bug 1228187. Part 1 - Add a throttling argument to MediaDecoderReader::NotifyDataArrived(). r=jya.

https://reviewboard.mozilla.org/r/26305/#review23867
Comment on attachment 8692680 [details]
MozReview Request: Bug 1228187. Part 2 - assert functions that shouldn't be called after Shutdown(). r=jya.

https://reviewboard.mozilla.org/r/26307/#review23869
Attachment #8692680 - Flags: review?(jyavenard) → review+
Attachment #8692679 - Flags: review+
Comment on attachment 8692679 [details]
MozReview Request: Bug 1228187. Part 1 - Add a throttling argument to MediaDecoderReader::NotifyDataArrived(). r=jya.

https://reviewboard.mozilla.org/r/26305/#review23871

note that this bug is no longer pressing as the computation times required to calculate the buffered range has been dropped to almost zero with bug 1227396.
Comment on attachment 8692679 [details]
MozReview Request: Bug 1228187. Part 1 - Add a throttling argument to MediaDecoderReader::NotifyDataArrived(). r=jya.

https://reviewboard.mozilla.org/r/26305/#review23873
Attachment #8692679 - Flags: review+
Do we still need this bug after bug 1227396 is landed? If performance is good enough, I would rather not to land this bug which makes the code a bit messy.
I guess it can be a wait & see.. however, calculating the buffered range every 32kB is still silly IMHO.
Priority: -- → P2
Looks like we don't need this bug anymore.
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: