Closed Bug 820588 Opened 7 years ago Closed 7 years ago

DASH: Fix statistics, especially download-rate for basic bandwidth estimation

Categories

(Core :: Networking, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla20

People

(Reporter: sworkman, Assigned: sworkman)

References

Details

Attachments

(1 file)

Current bandwidth estimation for DASH is based on download rate of an individual video stream (Bug 792404). Instead, it should be based on the average rate of all the video stream downloads. Other channel and playback statistics should also take sub streams into consideration.
-- UpdatePlaybackRate() after each byte range completes - non-DASH behavior is to update at the end of a full download, but with DASH only one stream will reach this point. This will update the media cache so it can estimate where the download is in relation to reading.

-- Keep playback stats in DASHRepDecoder - this is used by media cache to estimate playback rate of the current download of the current stream. It is appropriate only for a single sub-stream, not the average over several stream switches. So, it should be kept and updated in DASHRepDecoder. Therefore, DASHDecoder will forward calls to record Start and Stop times issued from the state machine.

-- Channel statistics should be considered as two aggregates for the audio and video streams. So, ChannelMediaResource is given a pointer to the appropriate channel statistics object, and records download stats there. This centralized object is then used to calculate download rates, and thus estimate bandwidth availability.

-- ComputePlaybackRate called from DASHDecoder uses the current video decoder. It would be nice to use an aggregate of audio and video, but the function GetStatistics records decoder and playback offsets, which cannot represent both audio and video positions. So, I opted to use video as the focus for these two functions.

-- Frame statistics are forwarded from the current video sub decoder to the main decoder in NotifyDecodedFrames.
Attachment #691093 - Flags: review?(cpearce)
Comment on attachment 691093 [details] [diff] [review]
v1.0 Aggregate and forward statistics in DASH decoder classes

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

Looks fine. It would be good if you could rename the instances of playbackRate that refer to the rate at which bytes are consumed for decode/playback to something else, so that we avoid confusion with the user-settable HTMLMediaElement.playbackRate which refers to the rate at which the playback clock runs.

::: content/media/MediaDecoder.h
@@ +617,5 @@
>    void StartProgressUpdates();
>  
>    // Something has changed that could affect the computed playback rate,
>    // so recompute it. The monitor must be held.
> +  virtual void UpdatePlaybackRate();

Instead of calling this "playback" rate, I'd rather call this "data decode" rate, or "data consumption" rate; we recently added support for HTMLMediaElement.playbackRate, which allows users/authors/Javascript to speed up the rate at which media is played back (unsurprisingly) and it would be good to not confuse these two concepts.

@@ +621,5 @@
> +  virtual void UpdatePlaybackRate();
> +
> +  // Used to estimate rates of data passing through the decoder's channel.
> +  // Records activity stopping on the channel. The monitor must be held.
> +  virtual void StartPlayback(TimeStamp aNow) {

We have a {Start/Stop}Playback functions on the MediaDecoderStateMachine, and they do something different to what you're doing here, so please rename these to something else appropriate to avoid confusion.

::: content/media/MediaDecoderStateMachine.cpp
@@ +1240,5 @@
>  
>    NS_ASSERTION(!IsPlaying(), "Shouldn't be playing when StartPlayback() is called");
>    mDecoder->GetReentrantMonitor().AssertCurrentThreadIn();
>    LOG(PR_LOG_DEBUG, ("%p StartPlayback", mDecoder.get()));
> +  mDecoder->StartPlayback(TimeStamp::Now());

Indeed, given how you're using these functions, you could just call them NotifyPlaybackStarted() etc, or StartRecordingChannelStatistics(), etc...

::: content/media/MediaResource.cpp
@@ +314,5 @@
>    }
>  
>    {
>      MutexAutoLock lock(mLock);
> +    mChannelStatistics->Start(TimeStamp::Now());

Do we ever not pass TimeStamp::Now() to ChannelStatistics::Start() and Stop()? Maybe we should just remove the parameter and calculate Now() inside these functions?

::: content/media/MediaResource.h
@@ +453,5 @@
>    virtual bool     CanClone();
>    virtual MediaResource* CloneData(MediaDecoder* aDecoder);
> +  // Set statistics to be recorded to the object passed in. If not called,
> +  // |ChannelMediaResource| will create it's own statistics objects in |Open|.
> +  void RecordStatisticsTo(MediaChannelStatistics *aStatistics)

Just put MOZ_OVERRIDE at the end of the line, not hanging low.
Attachment #691093 - Flags: review?(cpearce) → review+
(In reply to Chris Pearce (:cpearce) from comment #2)
> Comment on attachment 691093 [details] [diff] [review]
> v1.0 Aggregate and forward statistics in DASH decoder classes
> 
> Review of attachment 691093 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks fine. It would be good if you could rename the instances of
> playbackRate that refer to the rate at which bytes are consumed for
> decode/playback to something else, so that we avoid confusion with the
> user-settable HTMLMediaElement.playbackRate which refers to the rate at
> which the playback clock runs.

Thanks for the review. I've opened another bug for the 'playbackRate' changes … bug 821121.

> ::: content/media/MediaDecoder.h
> @@ +617,5 @@
> >    void StartProgressUpdates();
> >  
> >    // Something has changed that could affect the computed playback rate,
> >    // so recompute it. The monitor must be held.
> > +  virtual void UpdatePlaybackRate();
> 
> Instead of calling this "playback" rate, I'd rather call this "data decode"
> rate, or "data consumption" rate; we recently added support for
> HTMLMediaElement.playbackRate, which allows users/authors/Javascript to
> speed up the rate at which media is played back (unsurprisingly) and it
> would be good to not confuse these two concepts.

I'd rather do such refactoring in a separate bug, if you don't mind. I have some questions about how broad you want the changes to be, and I'd rather keep that separate from this patch. I'll open that one later today.

> @@ +621,5 @@
> > +  virtual void UpdatePlaybackRate();
> > +
> > +  // Used to estimate rates of data passing through the decoder's channel.
> > +  // Records activity stopping on the channel. The monitor must be held.
> > +  virtual void StartPlayback(TimeStamp aNow) {
> 
> We have a {Start/Stop}Playback functions on the MediaDecoderStateMachine,
> and they do something different to what you're doing here, so please rename
> these to something else appropriate to avoid confusion.

Renamed to NotifyPlaybackStart|Stopped.

> ::: content/media/MediaDecoderStateMachine.cpp
> @@ +1240,5 @@
> >  
> >    NS_ASSERTION(!IsPlaying(), "Shouldn't be playing when StartPlayback() is called");
> >    mDecoder->GetReentrantMonitor().AssertCurrentThreadIn();
> >    LOG(PR_LOG_DEBUG, ("%p StartPlayback", mDecoder.get()));
> > +  mDecoder->StartPlayback(TimeStamp::Now());
> 
> Indeed, given how you're using these functions, you could just call them
> NotifyPlaybackStarted() etc, or StartRecordingChannelStatistics(), etc...

Oh - great minds, I guess.

> ::: content/media/MediaResource.cpp
> @@ +314,5 @@
> >    }
> >  
> >    {
> >      MutexAutoLock lock(mLock);
> > +    mChannelStatistics->Start(TimeStamp::Now());
> 
> Do we ever not pass TimeStamp::Now() to ChannelStatistics::Start() and
> Stop()? Maybe we should just remove the parameter and calculate Now() inside
> these functions?

Yeah, I saw that in the code myself and wondered. I can make that change now.

> ::: content/media/MediaResource.h
> @@ +453,5 @@
> >    virtual bool     CanClone();
> >    virtual MediaResource* CloneData(MediaDecoder* aDecoder);
> > +  // Set statistics to be recorded to the object passed in. If not called,
> > +  // |ChannelMediaResource| will create it's own statistics objects in |Open|.
> > +  void RecordStatisticsTo(MediaChannelStatistics *aStatistics)
> 
> Just put MOZ_OVERRIDE at the end of the line, not hanging low.

D'oh. That was leftover when I had a long function name. Fixed.
Comment on attachment 691093 [details] [diff] [review]
v1.0 Aggregate and forward statistics in DASH decoder classes

https://hg.mozilla.org/integration/mozilla-inbound/rev/c926d32f8ace
https://hg.mozilla.org/mozilla-central/rev/c926d32f8ace
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
See Also: → 1367705
You need to log in before you can comment on or make changes to this bug.