559468 - An option or command to stop video/audio download

Status: RESOLVED WONTFIX

(Toolkit :: Video/Audio Controls, enhancement)

Description

[Marco Túlio Costa]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.2.3pre) Gecko/20100405 Ant.com Toolbar 2.0.1 Firefox/3.6.3plugin1 (.NET CLR 3.5.30729)
Build Identifier: 

In a website with a very long video/audio (eg. a tutorial), you sometimes just don't want to load the video/audio or want to stop at the middle.
A context menu option (or another form interaction) to stop downloading the video/audio would be nice.

Reproducible: Always



Expected Results:  
A way to stop video/audio downloading.

Comment 1

[Chris Pearce [:cpearce (Not reading bugmail)]]

Pressing escape stops all loads in the page, including all video/audio loads. Would probably be nice to have something more discoverable in the right click menu though.

Comment 3

[Chris Pearce [:cpearce (Not reading bugmail)]]

This is a controls issue, moving into video controls component...

Comment 4

[Paul Adenot (:padenot)]

Attachment: Patch v1

I've copied the behaviour of the Escape key using a context menu entry. Another option I've tried, is to remove the src attribute from the element.

As I was finding these solutions quite inelegant, I tried to use the SuspendLoad() and ResumeLoad() members of nsHTMLAudioElement, but I can't figure out how to call these methods from js.

I would need to add a method to the nsIDOMHTMLMediaElement.idl interface, but I guess it would then be a extension to the standard (which could be useful).

I've also tried to add a method to nsIDomWindowUtils.idl, but I haven't found a way to downcast safely a nsIDomElement to nsHTMLMediaElement.

Comment 5

[Stefan]

(In reply to comment #4)
> I've copied the behaviour of the Escape key using a context menu entry.

If the playback stops when it runs into end of data, the audio/video can neither be resumed not be replayed.

Comment 6

[Paul Adenot (:padenot)]

Indeed this solution is unsatisfactory, I'm trying to implement the real feature, but I'm quite stuck (see comment 4). I'll try to look again at this this weekend or on monday.

Comment 7

[Paul Adenot (:padenot)]

Attachment: Patch v2

I've used another approach, pausing the stream download and resuming it on demand.

I'm concerned by the two methods I added to the nsIDOMHTMLMediaElement.idl file, these methods aren't in the Media Element specification, but I couldn't figure another way to access native code from js. I guess they should be prefixed.

Comment 8

[Stefan]

(In reply to comment #7)
> I've used another approach, pausing the stream download and resuming it on
> demand.

After running into the end of a stopped stream you cannot slide through a video but the busy symbol (no animation) is displayed on a darkended still frame. Pause/Play button changes state but there is no play action actually performed (until the stream load is resumed).

What happens if the server does not support support partial GET?

Comment 9

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 537558 [details] [diff] [review]
Patch v2

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

In general, looks pretty promising!

* I think you need to ask for ui-review from boriss once we r+ this here.

* Note that the media cache can suspend the download if it thinks that it's got enough data to play through to the end given the current download rate. It should also resume download if the user starts playing and it decides it will need more data soon. It can also drop buffered ranges if it thinks they are unlikely to be needed again.

* When the download is finished (and so networkState==IDLE and currentTime != 0) we'll be showing the "resume download" menu item (if the we haven't got the entire media buffered, which is possible), but we can't acutally resume the download.

I think a better way to calculate showResumeDownload would be:
let showResumeDownload = onMedia && this.target.networkState == this.target.NETWORK_IDLE && !bufferedToEnd(this.target);

bufferedToEnd() can use HTMLMediaElement.buffered, currentTime, and duration to check to see if the last buffered range contains the currentTime and ends at duration (i.e. the media is buffered from the current playback position, right up until the end of media). If duration==Number.NaN || buffered.length==0, bufferedToEnd() should return false.

* Don't do the "if (buffered.length==1) ..." block. The media cache won't download data before the current playback position anyway, and this check is inaccurate if the media cache has dropped blocks in order to free up space, or the user seeks causing non-consecutive buffered ranges. Just rely on bufferedToEnd() as described above.

* Change "LOG(PR_LOG_DEBUG, ("Entering stopdownload"));" to "LOG(PR_LOG_DEBUG, ("%p Entering stopdownload", this));", and the same for the LOG(...) in resumedownload.

* Remove extra linebreak between nsHTMLMediaElement::StopDownload() and nsHTMLMediaElement::ResumeDownload().

* You'll need to figure out how to ensure the strings you're adding get localized. Do they get localized?

Comment 10

[Chris Pearce [:cpearce (Not reading bugmail)]]

You also need to rev the uuid on HTMLMediaElement IDL.

Comment 11

[cajbir (:cajbir)]

(In reply to comment #7)
> I'm concerned by the two methods I added to the nsIDOMHTMLMediaElement.idl
> file, these methods aren't in the Media Element specification, but I
> couldn't figure another way to access native code from js. I guess they
> should be prefixed.

They should be prefixed with 'Moz'. See the other prefixed method(s).

Comment 12

[Paul Adenot (:padenot)]

> 
> I think a better way to calculate showResumeDownload would be:
> let showResumeDownload = onMedia && this.target.networkState ==
> this.target.NETWORK_IDLE && !bufferedToEnd(this.target);
>

This causes the "Resume Download" entry to be visible at the loading of the page, i.e. before any downloading of the audio stream, for a media element that has the |preload=none| or |preload=metadata|, so I'm adding |this.target.currentTime != 0.0| in that test.

Allowing to download a stream without having to play could be a feature, though, but we should change the label of that entry (to something like "Start Download" or something).

Comment 13

[Paul Adenot (:padenot)]

Attachment: Patch v3

I updated the patch with Chris' remarks.

My main concerns about this feature are the behaviour the controls should adopt in several situation :
- If the download is paused, and the stream arrives at the end of the buffered audio, I expect the stream to pause. Then, if I click on play, I expect the stream to remain paused, because we don't have data. We could start buffering and playing, also.
- If the download is paused, and the user seeks in a non-buffered zone, I expect the stream to remain paused. At the moment, the stream remain paused, and a throbbler is showing, suggesting that something is going to happen.

Maybe someone from UI could guide me here.

Comment 14

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to comment #12)
> > 
> > I think a better way to calculate showResumeDownload would be:
> > let showResumeDownload = onMedia && this.target.networkState ==
> > this.target.NETWORK_IDLE && !bufferedToEnd(this.target);
> >
> 
> This causes the "Resume Download" entry to be visible at the loading of the
> page, i.e. before any downloading of the audio stream, for a media element
> that has the |preload=none| or |preload=metadata|, so I'm adding
> |this.target.currentTime != 0.0| in that test.
> 
> Allowing to download a stream without having to play could be a feature,
> though, but we should change the label of that entry (to something like
> "Start Download" or something).

This seems perfectly reasonable to me, desirable in fact. Then I don't have to do the "play then pause" dance to prebuffer a video.

Comment 15

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to comment #13)
> - If the download is paused, and the stream arrives at the end of the
> buffered audio, I expect the stream to pause. Then, if I click on play, I
> expect the stream to remain paused, because we don't have data. We could
> start buffering and playing, also.

By "stream", do you mean "media playback"?

Shouldn't the media cache resume the download when playback gets close to the end of buffered data? Is this not happening?

It sounds like in this case we're "potentially playing", so we shouldn't pause playback or stop playback in order to buffer if possible.
See the spec at: http://www.w3.org/TR/html5/video.html#potentially-playing for more details.

> - If the download is paused, and the user seeks in a non-buffered zone, I
> expect the stream to remain paused. At the moment, the stream remain paused,
> and a throbbler is showing, suggesting that something is going to happen.

Do you mean that you expect the download to remain paused, or playback to remain paused? I'd expect the download to resume after a seek; I'd guess playing after a seek is the more common case, and the user can suspend the download again if they want.

Comment 16

[Paul Adenot (:padenot)]

Attachment: Patch v1 : add a context menu entry to start intentionally the download

Comment 17

[Paul Adenot (:padenot)]

Attachment: Patch v4

This patch adds a Start Download menu entry, to intentionally start the download, and implements the stop download feature as discussed in this bug.

Behavior :
- If the download is stopped, and the user seeks, the download starts again (since the user probably wants to listen / watch the media).
- If some of the media is buffered, the audio stream is playing, and the download is paused, the download is restarted when more data is needed to continue the playback.

Comment 18

[Stefan]

(In reply to comment #17)
> Behavior :
> - If the download is stopped, and the user seeks, the download starts again
> (since the user probably wants to listen / watch the media).

I would not expect to continue the download when I seek in that part of the stream which is already on the computer. If you only seek (but not play) there's no need for resuming the download. If you seek and then play the case is already handles by the part below:

> - If some of the media is buffered, the audio stream is playing, and the
> download is paused, the download is restarted when more data is needed to
> continue the playback.

Yes, this is what I'd expect.

Comment 19

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 538269 [details] [diff] [review]
Patch v4

I like this feature. :) Thanks for working on it!

If you stop a download while a seek is in progress, the seek never finishes, and you can't play to resume the video (probably because of the controls). Maybe you need to check if mDecoder->IsSeeking() in MozStopDownload(), and defer the mDecoder->Suspend() until SeekCompleted() is called?


>diff --git a/content/html/content/public/nsHTMLMediaElement.h b/content/html/content/public/nsHTMLMediaElement.h
>--- a/content/html/content/public/nsHTMLMediaElement.h
>+++ b/content/html/content/public/nsHTMLMediaElement.h
>@@ -706,11 +706,17 @@ protected:
>+  // PR_TRUE if the download has been paused
>+  PRPackedBool mDownloadPaused;
>+
>+  // PR_TRUE if the user requested the start of the download
>+  PRPackedBool mDownloadStartRequested;

Full stops at the end of comments/sentences please.

>diff --git a/content/html/content/src/nsHTMLMediaElement.cpp b/content/html/content/src/nsHTMLMediaElement.cpp
>--- a/content/html/content/src/nsHTMLMediaElement.cpp
>+++ b/content/html/content/src/nsHTMLMediaElement.cpp
>@@ -1128,16 +1129,20 @@ NS_IMETHODIMP nsHTMLMediaElement::SetCur
>   // The media backend is responsible for dispatching the timeupdate
>   // event if it changes the playback position as a result of the seek.
>   LOG(PR_LOG_DEBUG, ("%p SetCurrentTime(%f) starting seek", this, aCurrentTime));
>   nsresult rv = mDecoder->Seek(clampedTime);
> 
>   // We changed whether we're seeking so we need to AddRemoveSelfReference
>   AddRemoveSelfReference();
> 
>+  // If the download was paused, restart it.
>+  if (mDownloadPaused)
>+    MozStartDownload();
>+

Restart the download *before* calling mDecoder->Seek(), as if the (asynchronous) seek starts before MozStartDownload() runs, the seek operation will try to seek on the suspended stream and fail.


>@@ -2095,23 +2102,29 @@ void nsHTMLMediaElement::UpdateReadyStat
>   if (aNextFrame != NEXT_FRAME_AVAILABLE) {
>     ChangeReadyState(nsIDOMHTMLMediaElement::HAVE_CURRENT_DATA);
>     if (!mWaitingFired && aNextFrame == NEXT_FRAME_UNAVAILABLE_BUFFERING) {
>       FireTimeUpdate(PR_FALSE);
>       DispatchAsyncEvent(NS_LITERAL_STRING("waiting"));
>       mWaitingFired = PR_TRUE;
>+      // If we don't have the next frame, and the download is paused, we should
>+      // restart the download to continue playback.
>+      if (mDownloadPaused) {
>+        MozStartDownload();
>+      }
>+    } else if (aNextFrame == NEXT_FRAME_UNAVAILABLE && mDownloadPaused) {
>+        MozStartDownload();

Indentation is off here.

When we're playing a media with a paused download, and playback reaches the end of the buffered data, we pause briefly before restarting playback (as we hit the condition you added above to begin playback again). Maybe we can eliminate this pause in most cases by resuming the download if we've got less than (say) 5 or 10 seconds of buffered data? You can use the buffered and current time attributes to determine this.

>@@ -2630,8 +2643,35 @@ void nsHTMLMediaElement::FireTimeUpdate(
>       (mLastCurrentTime != time &&
>        (mTimeUpdateTime.IsNull() ||
>         now - mTimeUpdateTime >= TimeDuration::FromMilliseconds(TIMEUPDATE_MS)))) {
>     DispatchAsyncEvent(NS_LITERAL_STRING("timeupdate"));
>     mTimeUpdateTime = now;
>     mLastCurrentTime = time;
>   }
> }
>+
>+nsresult nsHTMLMediaElement::MozStopDownload()
>+{
>+  if (mNetworkState == nsIDOMHTMLMediaElement::NETWORK_LOADING) {

Instead use:

  if (mNetworkState != nsIDOMHTMLMediaElement::NETWORK_LOADING)
    return NS_OK;
  
Then you save a level on indentation on the rest of the function.

>+    mDownloadPaused = PR_TRUE;
>+    mDecoder->Suspend();
>+    mNetworkState = nsIDOMHTMLMediaElement::NETWORK_IDLE;
>+    DispatchAsyncEvent(NS_LITERAL_STRING("suspend"));

You shouldn't need to set the network state and dispatch the suspend event here; mDecoder->Suspend() should call nsHTMLMediaElement::DownloadSuspended() which does that for you.

>+nsresult nsHTMLMediaElement::MozStartDownload()
>+{
>+  if (mNetworkState == nsIDOMHTMLMediaElement::NETWORK_IDLE) {

Instead use:

  if (mNetworkState != nsIDOMHTMLMediaElement::NETWORK_IDLE)
    return NS_OK;
  

>+    if (mDownloadPaused) {
>+      mDownloadPaused = PR_TRUE;

Did you mean: mDownloadPaused = PR_FALSE;  ?

If you:
1. Load a preload=auto video.
2. Stop the download.
3. Seek to almost end and play the video.

Then when playback reaches the end, the throbber shows indefinitely.

Setting mDownloadPaused to PR_FALSE here prevents that for me.

Comment 20

[Paul Adenot (:padenot)]

Attachment: Patch v5

I've addressed your concerns in this new version of the patch.

Comment 21

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 538529 [details] [diff] [review]
Patch v5

Thanks, almost there!

>diff --git a/browser/base/content/nsContextMenu.js b/browser/base/content/nsContextMenu.js
>--- a/browser/base/content/nsContextMenu.js
>+++ b/browser/base/content/nsContextMenu.js
>@@ -1309,16 +1312,27 @@ nsContextMenu.prototype = {
>     uri = uri.replace(/%LOCALE%/, escape(locale)).replace(/%VERSION%/, version);
> 
>     var newWindowPref = gPrefService.getIntPref("browser.link.open_newwindow");
>     var where = newWindowPref == 3 ? "tab" : "window";
> 
>     openUILinkIn(uri, where);
>   },
> 
>+  // return true if the stream is buffered to the end of the stream
>+  // i.e. we can play through the end without to download any more data
>+  bufferedToEnd : function(mediaElement) {
>+    if(mediaElement.duration == Number.NaN || mediaElement.buffered.length == 0)

Spacing doesn't match convention in file, space after "if" please.


>+      return false;
>+    let ranges = mediaElement.buffered;
>+    if(mediaElement.currentTime >= ranges.start(ranges.length - 1) && ranges.end(ranges.length - 1) == mediaElement.duration)

Space after "if".


>+      return true;
>+    return false;
>+  },
>+
>   bookmarkThisPage: function CM_bookmarkThisPage() {
>     window.top.PlacesCommandHook.bookmarkPage(this.browser, PlacesUtils.bookmarksMenuFolderId, true);
>   },
> 
>   bookmarkLink: function CM_bookmarkLink() {
>     window.top.PlacesCommandHook.bookmarkLink(PlacesUtils.bookmarksMenuFolderId, this.linkURL,
>                                               this.linkText());
>   },
>diff --git a/content/html/content/public/nsHTMLMediaElement.h b/content/html/content/public/nsHTMLMediaElement.h
>--- a/content/html/content/public/nsHTMLMediaElement.h
>+++ b/content/html/content/public/nsHTMLMediaElement.h
>@@ -208,16 +208,18 @@ public:
>     // The next frame of audio/video is unavailable because the decoder
>     // is paused while it buffers up data
>     NEXT_FRAME_UNAVAILABLE_BUFFERING,
>     // The next frame of audio/video is unavailable for some other reasons
>     NEXT_FRAME_UNAVAILABLE
>   };
>   void UpdateReadyStateForData(NextFrameStatus aNextFrame);
> 
>+  PRBool IsBuffered(const double aPosition);
>+

Add an appropriate comment on IsBuffered please. 


>diff --git a/content/html/content/src/nsHTMLMediaElement.cpp b/content/html/content/src/nsHTMLMediaElement.cpp
>--- a/content/html/content/src/nsHTMLMediaElement.cpp
>+++ b/content/html/content/src/nsHTMLMediaElement.cpp
>@@ -1112,32 +1117,38 @@ NS_IMETHODIMP nsHTMLMediaElement::SetCur
>   }
> 
>   // Detect for a NaN and invalid values.
>   if (aCurrentTime != aCurrentTime) {
>     LOG(PR_LOG_DEBUG, ("%p SetCurrentTime(%f) failed: bad time", this, aCurrentTime));
>     return NS_ERROR_FAILURE;
>   }
> 
>+  // If the download was paused and we are seeking to a zone that is not
>+  // buffered, restart it.
>+  if (mDownloadPaused && !IsBuffered(aCurrentTime))
>+    MozStartDownload();

Unfortunately you can't assume that IsBuffered(t) means you can then you can seek to t without needing other potentially unbuffered data. In the case of Ogg/Theora, the GetBuffered implementation doesn't guarantee that you have the keyframe buffered for all times that are in the buffered ranges. So if t is buffered, but it requires a keyframe at t-n which isn't buffered, the seek will still hang indefinintely. Best to just always restart the download until we make GetBuffered more precise.

Could we start the download if mDownloadPaused (i.e. drop the IsBuffered condition), and then set mDeferStopDownload=PR_TRUE? That would pause the download again after the seek completes, in SeekCompleted().

>+
>   // Clamp the time to [0, duration] as required by the spec
>   double clampedTime = NS_MAX(0.0, aCurrentTime);
>   double duration = mDecoder->GetDuration();
>   if (duration >= 0) {
>     clampedTime = NS_MIN(clampedTime, duration);
>   }
> 
>   mPlayingBeforeSeek = IsPotentiallyPlaying();
>   // The media backend is responsible for dispatching the timeupdate
>   // event if it changes the playback position as a result of the seek.
>   LOG(PR_LOG_DEBUG, ("%p SetCurrentTime(%f) starting seek", this, aCurrentTime));
>   nsresult rv = mDecoder->Seek(clampedTime);
> 
>   // We changed whether we're seeking so we need to AddRemoveSelfReference
>   AddRemoveSelfReference();
> 
>+
>   return rv;
> }

Don't add extra new line.

> 
> /* readonly attribute double duration; */
> NS_IMETHODIMP nsHTMLMediaElement::GetDuration(double *aDuration)
> {
>   *aDuration = mDecoder ? mDecoder->GetDuration() : std::numeric_limits<double>::quiet_NaN();
>   return NS_OK;
>@@ -1303,17 +1314,19 @@ nsHTMLMediaElement::nsHTMLMediaElement(a
>     mDelayingLoadEvent(PR_FALSE),
>     mIsRunningSelectResource(PR_FALSE),
>     mSuspendedAfterFirstFrame(PR_FALSE),
>     mAllowSuspendAfterFirstFrame(PR_TRUE),
>     mHasPlayedOrSeeked(PR_FALSE),
>     mHasSelfReference(PR_FALSE),
>     mShuttingDown(PR_FALSE),
>     mLoadIsSuspended(PR_FALSE),
>-    mMediaSecurityVerified(PR_FALSE)
>+    mMediaSecurityVerified(PR_FALSE),
>+    mDownloadPaused(PR_FALSE),
>+    mDownloadStartRequested(PR_FALSE)

Initialize mDeferStopDownload.

> {
> #ifdef PR_LOGGING
>   if (!gMediaElementLog) {
>     gMediaElementLog = PR_NewLogModule("nsMediaElement");
>   }
>   if (!gMediaElementEventsLog) {
>     gMediaElementEventsLog = PR_NewLogModule("nsMediaElementEvents");
>   }
>@@ -2053,16 +2066,20 @@ void nsHTMLMediaElement::SeekStarted()
>   FireTimeUpdate(PR_FALSE);
> }
> 
> void nsHTMLMediaElement::SeekCompleted()
> {
>   mPlayingBeforeSeek = PR_FALSE;
>   SetPlayedOrSeeked(PR_TRUE);
>   DispatchAsyncEvent(NS_LITERAL_STRING("seeked"));
>+  if(mDeferStopDownload) {

Space after "if" please.


>+    mDeferStopDownload = PR_FALSE;
>+    MozStopDownload();
>+  }
>   // We changed whether we're seeking so we need to AddRemoveSelfReference
>   AddRemoveSelfReference();
> }
> 
> void nsHTMLMediaElement::DownloadSuspended()
> {
>   if (mBegun) {
>     mNetworkState = nsIDOMHTMLMediaElement::NETWORK_IDLE;
>@@ -2095,46 +2112,74 @@ void nsHTMLMediaElement::UpdateReadyStat
> {
>   if (mReadyState < nsIDOMHTMLMediaElement::HAVE_METADATA) {
>     // aNextFrame might have a next frame because the decoder can advance
>     // on its own thread before ResourceLoaded or MetadataLoaded gets
>     // a chance to run.
>     // The arrival of more data can't change us out of this readyState.
>     return;
>   }
>-
>   if (aNextFrame != NEXT_FRAME_AVAILABLE) {
>     ChangeReadyState(nsIDOMHTMLMediaElement::HAVE_CURRENT_DATA);
>     if (!mWaitingFired && aNextFrame == NEXT_FRAME_UNAVAILABLE_BUFFERING) {
>       FireTimeUpdate(PR_FALSE);
>       DispatchAsyncEvent(NS_LITERAL_STRING("waiting"));
>       mWaitingFired = PR_TRUE;
>     }
>     return;
>   }
> 
>+  // See if are getting closed to the point the stream stopped buffering, to
>+  // restart the download if it has been paused.
>+  if(mDownloadPaused) {

Space after "if" please.


>+    double currentTime = 0.0;
>+    GetCurrentTime(&currentTime);
>+    if( ! IsBuffered(currentTime + RESTART_DOWNLOAD_S)) {

Spacing doesn't follow convention in that file, e.g. it should be:
if (!IsBuffered(currentTime + RESTART_DOWNLOAD_S)) {

>+      MozStartDownload();
>+    }
>+  }

Move the "if (mDownloadPaused)"  block up to between the "if (readyState < HAVE_METADATA)" block and the "if (aNextFrame != AVAILABLE)" block. Otherwise if we play when there's no data available, the download won't restart because of the early return in the "if (aNextFrame != AVAILABLE)" block. (STR: pause playback, stop download, seek, wait for seek to complete, play, observe the download and playback won't restart).

>+
>   // Now see if we should set HAVE_ENOUGH_DATA.
>   // If it's something we don't know the size of, then we can't
>   // make a real estimate, so we go straight to HAVE_ENOUGH_DATA once
>   // we've downloaded enough data that our download rate is considered
>   // reliable. We have to move to HAVE_ENOUGH_DATA at some point or
>   // autoplay elements for live streams will never play. Otherwise we
>   // move to HAVE_ENOUGH_DATA if we can play through the entire media
>   // without stopping to buffer.
>   nsMediaDecoder::Statistics stats = mDecoder->GetStatistics();
>   if (stats.mTotalBytes < 0 ? stats.mDownloadRateReliable :
>-                              stats.mTotalBytes == stats.mDownloadPosition ||
>+      stats.mTotalBytes == stats.mDownloadPosition ||
>       mDecoder->CanPlayThrough())
>   {
>     ChangeReadyState(nsIDOMHTMLMediaElement::HAVE_ENOUGH_DATA);
>     return;
>   }
>   ChangeReadyState(nsIDOMHTMLMediaElement::HAVE_FUTURE_DATA);
> }
> 
>+

Don't add extra newline between functions please.

>+PRBool nsHTMLMediaElement::IsBuffered(const double aPosition)
>+{
>+  nsIDOMTimeRanges* ranges;
>+  PRUint32 count = 0;
>+
>+  GetBuffered(&ranges);
>+  ranges->GetLength(&count);
>+  for (PRUint32 i = 0; i < count; i++) {
>+    double begin = 0.0;
>+    double end   = 0.0;
>+    ranges->Start(i, &begin);
>+    ranges->End(i, &end);
>+    if(begin < aPosition && aPosition < end)

Space after "if" please.

>+      return PR_TRUE;
>+  }
>+  return PR_FALSE;

You'll leak ranges here. Can you make ranges an nsCOMPtr? That should free it automatically. You'll probably need to use getter_Addrefs when calling GetBuffered.

>@@ -2630,8 +2675,36 @@ void nsHTMLMediaElement::FireTimeUpdate(
>       (mLastCurrentTime != time &&
>        (mTimeUpdateTime.IsNull() ||
>         now - mTimeUpdateTime >= TimeDuration::FromMilliseconds(TIMEUPDATE_MS)))) {
>     DispatchAsyncEvent(NS_LITERAL_STRING("timeupdate"));
>     mTimeUpdateTime = now;
>     mLastCurrentTime = time;
>   }
> }
>+
>+nsresult nsHTMLMediaElement::MozStopDownload()
>+{
>+  if (mNetworkState != nsIDOMHTMLMediaElement::NETWORK_LOADING) {
>+    return NS_OK;
>+  }
>+  if (mDecoder->IsSeeking()) {
>+    mDeferStopDownload = PR_TRUE;

You need to return NS_OK here inside the if condition, otherwise you will still suspend the download, and the seek will still hang.

>+  }
>+  mDownloadPaused = PR_TRUE;
>+  mDecoder->Suspend();
>+  return NS_OK;
>+}

Comment 22

[Chris Pearce [:cpearce (Not reading bugmail)]]

Why does this depend on bug 619752?

Comment 23

[Paul Adenot (:padenot)]

When server doesn't support range-request, the download / pause download changes, and this patch, as it is, was not showing proper context-menu. I guess I will be able to fix that when bug 619752 is fixed, i.e. we can easily detect when the server doesn't support range requests from the front-end (I made a few tests, and it is indeed better).

Comment 24

[Paul Adenot (:padenot)]

I wonder if setting the duration of the media from the buffered range when the download stops naturally (i.e. without using mozStopDownload) would be acceptable. It would be useful to distinguish fully buffered finite stream without duration information available (such as a media served without content-length header, on a server that doesn't accept range requests), from a webradio stream (both have only one range in |buffered|, can seek in that range, and have infinite duration).

It would be useful to implement the Start download context menu entry for unlimited streams.

Comment 25

[Stefan]

Patch v5 no longer applies to trunk (b7f03b37cf0c): 

1 out of 8 hunks FAILED -- saving rejects to file content/html/content/src/nsHTMLMediaElement.cpp.rej

1 out of 2 hunks FAILED -- saving rejects to file dom/interfaces/html/nsIDOMHTMLMediaElement.idl.rej

Comment 26

[Paul Adenot (:padenot)]

Attachment: Patch v0 - Fix browser/base/content/test/subtst_contextmenu.html test.

Comment 27

[Stefan]

Patch v5 does not apply to trunk (ed019d1cd8ec) due to a uid mismatch.

Comment 28

[Paul Adenot (:padenot)]

Yes, this patch is quite old. I'll update it (and finalize the front-end part) when I'm done with the current stuff I'm doing (likely by the end of the week).

Comment 29

[Jared Wein [:jaws] (please needinfo? me)]

Paul: Do you think you will be able to pick this bug up when you get your new computer?

Comment 30

[Paul Adenot (:padenot)]

Jared : definitely, it should be a matter of days as for now.

Comment 31

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 538529 [details] [diff] [review]
Patch v5

I look forward to seeing an updated version of the patch when you get the time Paul. :)

Comment 32

[Paul Adenot (:padenot)]

Attachment: v6 - rebased and proper event firing.

I've rebased the patch, and added (what I think is) proper event firing :
- Suspend when the download is paused ;
- Progress when the download is restarted ;
- No progress events when the download is stopped.

Comment 33

[Paul Adenot (:padenot)]

Attachment: Test for mozStopDownload

Here is a test for the feature, and a green try : https://tbpl.mozilla.org/?tree=Try&rev=5edea561c8b8

Comment 34

[Paul Adenot (:padenot)]

Attachment: Fix test_contextmenu.html

Comment 35

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 619179 [details] [diff] [review]
v6 - rebased and proper event firing.

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

You need to split this patch up into /browser/* changes and Gecko changes, and get someone in the Firefox team to review the browser/* changes/. Perhaps Jared Wein (:jaws).

You should also restart the download if the user seeks. Currently if you: play, pause, stop download and then seek, the seek never completes. I think it makes sense *not* to defer the stop download in this case, i.e. seeking should restart the download, not stop again after seeking.

There may be other permutations of this that cause problems too. Play around. You may find my rate-controlled webserver at https://github.com/cpearce/HttpMediaServer useful in testing this.

I built your patches, and I was also able to get the context menu into a state where it wasn't toggling, and was stuck on "Start download" no matter what I did. I think this could be caused by the seeking problem above.

::: content/html/content/public/nsHTMLMediaElement.h
@@ +228,5 @@
>      NEXT_FRAME_UNAVAILABLE
>    };
>    void UpdateReadyStateForData(NextFrameStatus aNextFrame);
>  
> +  // Return True if aPosition refers to a buffered range in media stream

s/Return True/Returns true/ and add a fullstop at the end of sentence please.

::: content/html/content/src/nsHTMLMediaElement.cpp
@@ +2482,5 @@
>    // move to HAVE_ENOUGH_DATA if we can play through the entire media
>    // without stopping to buffer.
>    nsMediaDecoder::Statistics stats = mDecoder->GetStatistics();
>    if (stats.mTotalBytes < 0 ? stats.mDownloadRateReliable :
> +      stats.mTotalBytes == stats.mDownloadPosition ||

Leave the original indentation.

@@ +2503,5 @@
> +    double begin = 0.0;
> +    double end   = 0.0;
> +    ranges->Start(i, &begin);
> +    ranges->End(i, &end);
> +    if (begin < aPosition && aPosition < end)

if (begin <= aPosition && aPosition <= end)

(otherwise, IsBuffered(duration) returns false, right?)

@@ +3015,5 @@
>  }
>  
> +nsresult nsHTMLMediaElement::MozStopDownload()
> +{
> +  if (mNetworkState != nsIDOMHTMLMediaElement::NETWORK_LOADING) {

Better also return early here if !mDecoder, otherwise you could have a null pointer deref if mDecoder isn't yet initialized. I'm not sure if that's possible, but better off being safe. Returning NS_OK if !mDecoder is fine I think.

@@ +3030,5 @@
> +}
> +
> +nsresult nsHTMLMediaElement::MozStartDownload()
> +{
> +  if (mNetworkState != nsIDOMHTMLMediaElement::NETWORK_IDLE) {

Add mDecoder null check.

::: content/media/nsBuiltinDecoder.cpp
@@ +682,5 @@
>      UpdatePlaybackRate();
>    }
>  
>    if (NS_SUCCEEDED(aStatus)) {
> +    if (IsInfinite()) {

Why do you need to do this change? If we stop download on a live stream, what happens? If we restart download on a live stream, what happens?

@@ +968,5 @@
>  void nsBuiltinDecoder::Suspend()
>  {
>    NS_ASSERTION(NS_IsMainThread(), "Should be on main thread.");
>    if (mResource) {
> +    ReentrantMonitorAutoEnter mon(mReentrantMonitor);

I don't think you need to take the montior here?

@@ +978,5 @@
>  void nsBuiltinDecoder::Resume(bool aForceBuffering)
>  {
>    NS_ASSERTION(NS_IsMainThread(), "Should be on main thread.");
>    if (mResource) {
> +    ReentrantMonitorAutoEnter mon(mReentrantMonitor);

I don't think you need to take the montior here?

::: content/media/nsBuiltinDecoderStateMachine.cpp
@@ +2283,5 @@
>      return NS_OK;
>    }
>  
>    mTimeout = timeout;
> +   

Don't add whitespace please.

::: dom/interfaces/html/nsIDOMHTMLMediaElement.idl
@@ +122,5 @@
>    // buffered data. In fact any future data downloaded by this element or
>    // other will be sharable by both elements.
>    void mozLoadFrom(in nsIDOMHTMLMediaElement other);
>  
> +  // Mozilla extension: stop the download of the source, iff the network

Rev the uuid of Media, Audio and Video elements.

I told you about http://people.mozilla.org/~sfink/uploads/update-uuids right?

Comment 36

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 619180 [details] [diff] [review]
Test for mozStopDownload

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

Almost there, but I think the 2s timeout would be prone to random failures.

::: content/media/test/test_mozStopDownload.html
@@ +31,5 @@
> +    t.removeEventListener('suspend', onprogress_stopped);
> +    t.addEventListener('progress', onprogress_restartDownload);
> +    t.mozStartDownload();
> +    is(t.networkState, t.NETWORK_LOADING, "NetworkState should be NETWORK_LOADING.");
> +  }, 2000);

Rather than sleeping for 2s, can you have your progress and suspend listeners each increment counters to say how many times they've fired, and have them check to see if both fired at least once and then continue the test if so? That'll prevent random failures in case the events hadn't managed to fire in 2s.

@@ +54,5 @@
> +  is(t.networkState, t.NETWORK_LOADING, "NetworkState should be NETWORK_LOADING.");
> +  t.removeEventListener('progress', onprogress_restartDownload);
> +  // Do a request again to stop the server.
> +  t.src = 'dummy.ogg';
> +  t.src = 'slow_server.sjs';

Does setting t.src="slow_server.sjs" restart the connection?

Would setting t.src="" dispose of the decoder's network connections and thus stop the server?

Comment 37

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 619181 [details] [diff] [review]
Fix test_contextmenu.html

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

This isn't my area. Merge the changes in this patch into the browser/* patch I asked you to make in my earlier comments, and ask Jared Wein to review it. Thanks!

Comment 38

[Paul Adenot (:padenot)]

> I built your patches, and I was also able to get the context menu into a
> state where it wasn't toggling, and was stuck on "Start download" no matter
> what I did. I think this could be caused by the seeking problem above.
I could reproduce using the |live| option of you HttpMediaServer, and I've addressed that in the frontend patch.
 
> ::: content/media/nsBuiltinDecoder.cpp
> @@ +682,5 @@
> >      UpdatePlaybackRate();
> >    }
> >  
> >    if (NS_SUCCEEDED(aStatus)) {
> > +    if (IsInfinite()) {
> 
> Why do you need to do this change? If we stop download on a live stream,
> what happens? If we restart download on a live stream, what happens?

This is an attempt to give the user a duration when the media is downloaded in its entirety, and when it's not a live stream, but a regular media served without ranges. If the user play the media, and wants to play it again, it should be possible to seek around without problem. Since it was impossible avoid downloading the entire media (because GetSeekable was returning GetBuffered), we will not have to re-download part of the media, which would be impossible because the server does not support range requests.

Don't know if it's worth it or if we want that behavior, it just thought it would be useful and technically possible.

Comment 39

[Paul Adenot (:padenot)]

> @@ +54,5 @@
> > +  is(t.networkState, t.NETWORK_LOADING, "NetworkState should be NETWORK_LOADING.");
> > +  t.removeEventListener('progress', onprogress_restartDownload);
> > +  // Do a request again to stop the server.
> > +  t.src = 'dummy.ogg';
> > +  t.src = 'slow_server.sjs';
> 
> Does setting t.src="slow_server.sjs" restart the connection?

Nope.

> Would setting t.src="" dispose of the decoder's network connections and thus
> stop the server?

I could not manage to close the connection in a way that `slow_server.js` is notified, to be able to cancel the timer. There is no API call I can use in httpd.js to get the status of the underlying channel, once |processAsync| has been called.

Comment 40

[Paul Adenot (:padenot)]

Attachment: v7 - Addressed comments.

Comment 41

[Paul Adenot (:padenot)]

Attachment: Fixed behavior for live streams.

Comment 42

[Paul Adenot (:padenot)]

Attachment: Addressed comments.

Comment 43

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 620898 [details] [diff] [review]
Fixed behavior for live streams.

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

Overall looks good, but I've got some questions about a few parts. Nothing major though.

::: browser/base/content/nsContextMenu.js
@@ +440,5 @@
>  
> +    // When the stream is infinite (live stream or media served without ranges),
> +    // we allow to stop the download, but it will be restarted automatically if
> +    // needed (when the |currentTime| reaches the end of the buffered data).
> +    let showStartDownload = onMedia && this.target.networkState == this.target.NETWORK_IDLE && !this.bufferedToEnd(this.target) && this.target.duration != Infinity;

I think it is nice that we will restart a live stream for the user if they run out of buffered media, but I think we should still provide the user the ability to resume loading before the end of the buffered range is reached. Can you remove the extra condition of |this.target.duration != Infinity|?

@@ +1409,5 @@
>    },
>  
> +  // return true if the stream is buffered to the end of the stream
> +  // i.e. we can play the end without having to download any more data
> +  bufferedToEnd : function(mediaElement) {

This function name is a little misleading. So it will only return true if the buffer reaches the end of the media (as it is titled) but also only if the currentTime is within said buffer. Can you update the comment to this function to say "Returns true if no new buffers are needed between the current time and the end of the media."

::: browser/base/content/test/test_contextmenu.html
@@ +355,5 @@
>  
>      case 8:
>          // Context menu for a video (with a VALID media source)
> +        // If the file is fully buffered, startdownload menu entry is not
> +        // showed.

s/showed/shown

@@ +381,5 @@
> +                            "context-video-fullscreen",   true,
> +                            "---",                        null,
> +                            "context-viewvideo",          true,
> +                            "context-copyvideourl",       true,
> +                            "context-startdownload",      true,

This is startdownload because the <video> tag doesn't have [preload], correct? If so, shouldn't this always enter the 'startdownload' case, or does the video load completely sometimes and not completely other times?

::: browser/locales/en-US/chrome/browser/browser.dtd
@@ +435,5 @@
>  <!ENTITY copyVideoURLCmd.label        "Copy Video Location">
>  <!ENTITY copyVideoURLCmd.accesskey    "o">
>  <!ENTITY copyAudioURLCmd.label        "Copy Audio Location">
>  <!ENTITY copyAudioURLCmd.accesskey    "o">
> +<!ENTITY startDownloadCmd.label           "Start Download">

I think we'll need some better wording here, so users don't confuse this with Save Video. Can we use "Start Loading" and "Stop Loading", respectively?

Comment 44

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 620898 [details] [diff] [review]
Fixed behavior for live streams.

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

::: browser/base/content/nsContextMenu.js
@@ +440,5 @@
>  
> +    // When the stream is infinite (live stream or media served without ranges),
> +    // we allow to stop the download, but it will be restarted automatically if
> +    // needed (when the |currentTime| reaches the end of the buffered data).
> +    let showStartDownload = onMedia && this.target.networkState == this.target.NETWORK_IDLE && !this.bufferedToEnd(this.target) && this.target.duration != Infinity;

So I built your new patches, and I get weird behaviour with live streams. :(

When I load a live stream, the "Stop download" option is visible. But if I run it, the option vanishes, and furthermore there's no "Start download" option. Despite the download being "stopped", I can keep playing.

So it seems that on live streams "Stop download" doesn't actually have any observable affect (with our default controls), since (with our default controls) I can't see how far ahead we're buffered when playing a live stream, and playing keeps working.

However if you stop the download on a live stream and keep playing long enough to reach the end of buffered data, playback halts there and doesn't restart. I just get the throbber showing perpetually once playback reaches the end of bufferd data.

So it seems to me that if we have a "Stop download" option on live streams, it should behave the same if the user presses ESC, i.e. it renders the media unplayable. Or we could have "Start download" reload the media on infinite streams (if they're true "live" streams, this would make perfect sense). Or we could just not have the stop/start download options on live streams.

Comment 45

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 620897 [details] [diff] [review]
v7 - Addressed comments.

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

Getting closer. ;)

::: content/html/content/src/nsHTMLMediaElement.cpp
@@ +2743,5 @@
>      return;
>    }
>  
> +  // See if are getting closed to the point the stream stopped buffering, to
> +  // restart the download if it has been paused.

This could be worded better. Make that:

  // If the download has been paused and we are getting closer to the point
  // the stream stopped buffering, restart the download.

::: content/media/nsBuiltinDecoder.cpp
@@ +712,5 @@
>      UpdatePlaybackRate();
>    }
>  
>    if (NS_SUCCEEDED(aStatus)) {
> +    if (IsInfinite()) {

When stopping the download by pressing ESC, this code didn't appear to have any effect; duration is still +Inf, because mInfiniteStream is still true.

I don't think we should make this change, because if a live stream is stopped, it's still *not* the same as a non-live stream, which is what this change makes it appear. The media cache is free to discard parts of the media which have been played, so since on live streams we can't seek into discarded regions, we can't guarantee that seeks can be made into the entire media.

@@ -1008,4 @@
>      mResource->Resume();
>    }
>    if (aForceBuffering) {
> -    ReentrantMonitorAutoEnter mon(mReentrantMonitor);

We actually still need *this* monitor acquisition, as the call to StartBuffering() below asserts that the monitor is held in debug builds leading to a fatal assertion...

What I meant was, you don't need to hold the monitor for the {Start,Stop}Progress() calls.

Comment 46

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to Chris Pearce (:cpearce) from comment #44)
> Comment on attachment 620898 [details] [diff] [review]
> Fixed behavior for live streams.
> 
> Review of attachment 620898 [details] [diff] [review]:

Hmm, I didn't say that as clear as I could, let me try again:

I think that since we can't determine whether we can resume an HTTP download of an infinite ("live") media, when playing a live stream, the "Stop download" option should cancel the download, and pause playback. "Start download" should reload the media (video.src = video.currentSrc; should do it I think), and starting playing the media again.

In fact, playing a "stopped download" live stream should probably just reload the stream as well begin playback.

That seems more intuitive to me and more reliable. Jared, what do you think?

I also agree with Jared, we should change the strings... "Stop streaming" and "Restart streaming" in the live case maybe?

Comment 47

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Chris Pearce (:cpearce) from comment #46) 
> I think that since we can't determine whether we can resume an HTTP download
> of an infinite ("live") media, when playing a live stream, the "Stop
> download" option should cancel the download, and pause playback. "Start
> download" should reload the media (video.src = video.currentSrc; should do
> it I think), and starting playing the media again.
> 
> In fact, playing a "stopped download" live stream should probably just
> reload the stream as well begin playback.
> 
> That seems more intuitive to me and more reliable. Jared, what do you think?

Yeah, I think that makes sense, but I think if duration != INFINITE, then when reloading the stream we should also set reassign currentTime so the video begins playing and loading from where it left off at, but would that mean that resuming the load would lose any previously loaded ranges?

Comment 48

[Paul Adenot (:padenot)]

Jared, I think Chris was talking about live stream for this new behavior. If we can do range requests (if duration != Infinity), then we are not reloading the stream, we just suspend the download. That way, we do not have to reassign |currentTime|.

I'll implement the following :

- If the stream is not infinite, normal behavior, we can pause and restart the download. We have got the option in the context menu according to the |networkState| and |buffered| members. The label on the context menu entries are "Stop download" and "Start download".

- If the stream is infinite, we can stop the download. This pauses the stream and calls |mozStopDownload|. Then we can restart the stream. This does |media.src = media.currentSrc|, that restarts the download, and calls |play()|. The label on the context menu are "Stop streaming" and "Start streaming".

Would this behavior be acceptable ?

Comment 49

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 620899 [details] [diff] [review]
Addressed comments.

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

Almost there.

::: content/media/test/slow_server.sjs
@@ +33,5 @@
> +  response.processAsync();
> +
> +  timer = Components.classes["@mozilla.org/timer;1"].createInstance(Components.interfaces.nsITimer);
> +  timer.initWithCallback(function() {
> +    dump("blah\n");

Remove dumps please.

::: content/media/test/test_mozStopDownload.html
@@ +20,5 @@
> +  var t = e.target;
> +  t.addEventListener('progress', onprogress_stopped);
> +  t.removeEventListener('progress', onprogress);
> +  t.addEventListener("stalled", onstalled);
> +  t.addEventListener("suspend", onsuspend);

You're adding two "suspend" and two "progress" listeners here... How about you just call resume_download(e) in your onsuspend and onprogress listeners? Otherwise you'll receive two calls for each suspend/progress event dispatched by gecko.

I'd rename resume_download to maybe_resume_download() then to better reflect its purpose.

@@ +35,5 @@
> +  }
> +  is(t.gotProgress, true, "We should have received a \'progress\' when the download is suspended.");
> +  is(t.gotSuspend, true, "We should have received a \'suspend\' when the download is suspended.");
> +  t.removeEventListener('progress', onprogress_stopped);
> +  t.removeEventListener('suspend', onprogress_stopped);

You're removing the wrong listener here.

@@ +57,5 @@
> +}
> +
> +function onprogress_restartDownload(e) {
> +  var t = e.target;
> +  is(t.networkState, t.NETWORK_LOADING, "NetworkState should be NETWORK_LOADING.");

If the resource manages to complete playback before this fires, networkState will be IDLE. This may be unlikely given that you're using slow_server.sjs, but if it's possible that it can fail, you can bet it'll fail on tinderbox. ;)

So better make this:

ok(t.networkState == t.NETWORK_LOADING || t.ended, ...

Comment 50

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Paul ADENOT (:padenot) from comment #48)
> - If the stream is not infinite, normal behavior, we can pause and restart
> the download. We have got the option in the context menu according to the
> |networkState| and |buffered| members. The label on the context menu entries
> are "Stop download" and "Start download".
> 
> Would this behavior be acceptable ?

These should be "Stop Loading" and "Start Loading" as mentioned earlier to try to reduce confusion between downloading and saving.

Comment 51

[Paul Adenot (:padenot)]

> @@ +381,5 @@
> > +                            "context-video-fullscreen",   true,
> > +                            "---",                        null,
> > +                            "context-viewvideo",          true,
> > +                            "context-copyvideourl",       true,
> > +                            "context-startdownload",      true,
> 
> This is startdownload because the <video> tag doesn't have [preload],
> correct? If so, shouldn't this always enter the 'startdownload' case, or
> does the video load completely sometimes and not completely other times?

If we are not specifying a preload attribute, then it defaults to 'metadata' on desktop (at least on Linux, don't know about other platforms), and 'none' on mobile (but I suppose we are not testing context menu on mobile, so it should not matter). If the media is small enough, it is possible that it gets fully buffered when the metadata are decoded (and our test files are quite small), hence this particular case. I remember having tests failure on try a while ago without this.

Comment 52

[Paul Adenot (:padenot)]

I think we need a new boolean attribute |mozDownloadPaused| (I welcome better name suggestions, of course), to address the case when :

- the content is not served with ranges and ;
- we don't have duration information in the file and ;
- the media file has a finite duration (that is, it is not a live stream) and ;
- the media is fully buffered.

In this situation :
- duration == Infinity ;
- |buffered| cannot be compared to |duration| to see if the download seems to be complete ;
- networkState == NETWORK_IDLE.

that is, either the download is stopped (with mozStopDownload) on a live stream, or we have buffered the totality of a media served without ranges.

The semantic of this new member would be something along the line of "True if the download has intentionally been stopped".

Comment 53

[Paul Adenot (:padenot)]

Attachment: v8 addressed comments.

Comment 54

[Paul Adenot (:padenot)]

Attachment: Frontend part.

Comment 55

[Paul Adenot (:padenot)]

Attachment: v7 - Addressed comments.

Comment 56

[Paul Adenot (:padenot)]

Attachment: Tests.

Comment 57

[Paul Adenot (:padenot)]

Attachment: Right patch.

Comment 58

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 624561 [details] [diff] [review]
Frontend part.

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

Looks good. I'd like to see it again though before I r+.

::: browser/base/content/browser-context.inc
@@ +153,5 @@
>        <menuitem id="context-copyaudiourl"
>                  label="&copyAudioURLCmd.label;"
>                  accesskey="&copyAudioURLCmd.accesskey;"
>                  oncommand="gContextMenu.copyMediaLocation();"/>
> +      <menuitem id="context-startdownload"

These should be context-media-startdownload, context-media-stopdownload, etc.

::: browser/base/content/nsContextMenu.js
@@ +442,5 @@
> +    let showStartDownload = onMedia && !isLivestream && this.target.networkState == this.target.NETWORK_IDLE && !this.bufferedToEnd(this.target);
> +    this.showItem("context-startdownload", showStartDownload);
> +    this.showItem("context-stopdownload", onMedia && !isLivestream && (this.target.networkState == this.target.NETWORK_LOADING));
> +    this.showItem("context-startstreaming", onMedia && this.target.mozDownloadPaused && isLivestream && (this.target.networkState == this.target.NETWORK_IDLE));
> +    this.showItem("context-stopstreaming", onMedia && isLivestream && (this.target.networkState == this.target.NETWORK_LOADING));

nit: please wrap these lines to 80 characters so it is easier to review.

::: browser/base/content/test/test_contextmenu.html
@@ +395,5 @@
>  
>      case 9:
>          // Context menu for a video (with an audio-only file)
> +        // If the file is fully buffered, startdownload menu entry is not
> +        // showed.

s/showed/shown

::: browser/locales/en-US/chrome/browser/browser.dtd
@@ +461,5 @@
> +<!ENTITY startDownloadCmd.label           "Start Loading">
> +<!ENTITY startDownloadCmd.accesskey       "s">
> +<!ENTITY stopDownloadCmd.label           "Stop Loading">
> +<!ENTITY stopDownloadCmd.accesskey       "s">
> +<!ENTITY startStreamingCmd.label           "Start Streaming">

If the media.buffered.length > 0, then I think these strings should be "Resume loading" and "Resume Streaming".

Comment 59

[Paul Adenot (:padenot)]

Attachment: Addressed :jaws comments.

Comment 60

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 624952 [details] [diff] [review]
Addressed :jaws comments.

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

::: browser/base/content/nsContextMenu.js
@@ +474,5 @@
>        this.setItemAttr("context-media-unmute", "disabled", hasError);
>        this.setItemAttr("context-media-showcontrols", "disabled", hasError);
>        this.setItemAttr("context-media-hidecontrols", "disabled", hasError);
> +      this.setItemAttr("context-stopdownload", "disabled", hasError);
> +      this.setItemAttr("context-startdownload", "disabled", hasError);

These don't seem right. Shouldn't they be "context-media-stopdownload" and "context-media-startdownload", respectively?

@@ +1436,5 @@
> +    if (mediaElement.duration == Number.NaN ||
> +        mediaElement.duration == Number.Infinity ||
> +        mediaElement.buffered.length == 0)
> +      return false;
> +    let ranges = mediaElement.buffered;

Move the |let ranges = mediaElement.buffered| to the top of this function so we don't have to call mediaElement.buffered twice.

@@ +1550,5 @@
> +  },
> +
> +  stopDownload : function() {
> +    if (this.target.duration == Infinity) {
> +      this.target.pause();

Can you move backwards on a live stream? If so, should stopping the loading of the stream cause the video to pause? There could be a buffered range that is still playable if we allow moving backwards.

Comment 61

[Paul Adenot (:padenot)]

> @@ +1550,5 @@
> > +  },
> > +
> > +  stopDownload : function() {
> > +    if (this.target.duration == Infinity) {
> > +      this.target.pause();
> 
> Can you move backwards on a live stream? If so, should stopping the loading
> of the stream cause the video to pause? There could be a buffered range that
> is still playable if we allow moving backwards.

Yes we can, except for WebM (it was possible, but should not have been, fixed in bug 756372). With the current patch, if we click the "Stop Streaming" entry, then it pauses the video and the download, but we can still play in the already buffered ranges (on a theora video, for example). The cache can still drop buffered ranges when it wants, though.

FWIW, comment 46 says :

> when playing a live stream, the "Stop download" option should cancel
> the download, and pause playback.

I think this is reasonable. To me, streaming implies simultaneous downloading and presentation of a media, so "Stop Streaming" should stop both.

Comment 62

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Paul ADENOT (:padenot) from comment #61)
> FWIW, comment 46 says :
> 
> > when playing a live stream, the "Stop download" option should cancel
> > the download, and pause playback.
> 
> I think this is reasonable. To me, streaming implies simultaneous
> downloading and presentation of a media, so "Stop Streaming" should stop
> both.

The user can still hit the play button here. What should the user expect in that scenario? I'm still not sure why we are pausing the video in this case. If we run out of our buffer, then the playback should stop naturally.

Are we calling |pause| to workaround some issue with the video controls?

Comment 63

[Paul Adenot (:padenot)]

> The user can still hit the play button here. What should the user expect in that scenario? 

As a user, I expect the stream to restart (that is restarting the stream), which is not what is currently done. Hopefully this is easy with |mozDownloadStopped|, I'm going to implement that before resubmitting the patch.

> If we run out of our buffer, then the playback should stop naturally.

If we a playing close to the end of a buffered range, the existing logic will try to restart the download, which is impossible, because either the media is not served with ranges (that is, we have to start the download from the beginning), or it is a live stream.

If you think not calling |pause|, but checking if the stream is infinite in the underlying implementation, to end the playback at the end of the buffered range is better, I can do that.

Comment 64

[Chris Pearce [:cpearce (Not reading bugmail)]]

I built your patches and found the following problems:

Problem 1 STR:
1. Load and play a live resource.
2. "Stop Streaming" the media.
3. Seek to near end of resource (I executed var v=document.getElementsByTagName("video")[0];v.currentTime = v.buffered.end(v.buffered.length-1) - 10; in the webconsole).
4. Once playback reaches the end of resource, the play/pause button on the controls is still in playing state (i.e. it shows the "pause" button icon), but clicking on it has no affect. Additionally if you seek back to v.currentTime = v.buffered.end(v.buffered.length-1) - 10; again, the throbber shows and the resource can't be replayed.

We should be reloading the resource to restart playback in this case.


Problem 2 STR:
1. Load and play a non-live resource.
2. "Stop loading" the media.
3. Seek to near the end of the resource using |var v=document.getElementsByTagName("video")[0];v.currentTime = v.buffered.end(v.buffered.length-1) - 10;| in the webconsole.
4. Then when playback reaches the end of the resource it doesn't restart/continue playing. The play/pause button is still showing a pause icon, but toggling it has no effect, and selecting "Resume Loading" has no effect. There's no way to resume playback of the resource.

We should be able to resume download and playback of the resource in this case.

I'll hold of reviewing your /content/ patches until these problems are resolved.

Comment 65

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 624646 [details] [diff] [review]
Right patch.

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

::: content/media/test/test_mozStopDownload.html
@@ +9,5 @@
> +  <script type="text/javascript" src="/MochiKit/Signal.js"></script>
> +  <script type="text/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
> +  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css" />
> +</head>
> +<body onunload="mediaTestCleanup();">

mediaTestCleanup() is defined in manifest.js, which you're not including here? Can you just remove this call?

Comment 66

[Paul Adenot (:padenot)]

Attachment: Part 1 - An option or command to stop video/audio download (feature implementation) r=

Comment 67

[Paul Adenot (:padenot)]

Attachment: Part 2 - Front-end part. r=

Comment 68

[Paul Adenot (:padenot)]

I have reworked those patch a while ago, but never uploaded them. They address all the issue mentioned previously, and I tried to carefully test everything.

Basically, they change a bunch of behavior from the front-end code to the nsHTMLMediaElement, and address edge cases.

Comment 69

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 649929 [details] [diff] [review]
Part 2 - Front-end part. r=

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

::: browser/locales/en-US/chrome/browser/browser.dtd
@@ +481,5 @@
> +<!ENTITY resumeDownloadCmd.accesskey       "L">
> +<!ENTITY stopDownloadCmd.label           "Stop Loading">
> +<!ENTITY stopDownloadCmd.accesskey       "L">
> +<!ENTITY resumeDownloadCmd.label           "Resume Loading">
> +<!ENTITY resumeDownloadCmd.accesskey       "L">

It looks like resumeDownloadCmd.label + resumeDownloadCmd.accesskey are defined twice here.

Comment 70

[Paul Adenot (:padenot)]

I don't really have time to work on this in the near future.

I'll mark it as a mentored bug.

Comment 73

[Jared Wein [:jaws] (please needinfo? me)]

We don't have plans to support this, and we are actively looking for ways to simplify our controls and removed less used features, so adding this right now would be counter-intuitive.