Last Comment Bug 762282 - After having finished the download of an OGG video, networkState is NETWORK_LOADING.
: After having finished the download of an OGG video, networkState is NETWORK_L...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Audio/Video (show other bugs)
: Trunk
: x86_64 Linux
: -- normal (vote)
: mozilla17
Assigned To: Paul Adenot (:padenot)
:
: Maire Reavy [:mreavy]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-06-06 15:39 PDT by Paul Adenot (:padenot)
Modified: 2012-08-17 19:22 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v0: special case the OGG seek. (6.03 KB, patch)
2012-06-06 16:52 PDT, Paul Adenot (:padenot)
no flags Details | Diff | Splinter Review
Patch v1: don't forget to initialize the new member in the constructor. (6.61 KB, patch)
2012-06-21 14:52 PDT, Paul Adenot (:padenot)
cajbir.bugzilla: review+
Details | Diff | Splinter Review
Don't notify the HTMLMediaElement if we reach the end of the media while seeking for metadata. (6.83 KB, patch)
2012-08-17 10:01 PDT, Paul Adenot (:padenot)
padenot: review+
Details | Diff | Splinter Review

Description Paul Adenot (:padenot) 2012-06-06 15:39:22 PDT
This bug is intermittent, it takes me 3-4 times to reproduce. It seems easier to reproduce if the download speed is very high (but not too high like a local webserver).

STR:
- Load a OGG video (http://upload.wikimedia.org/wikipedia/commons/e/e6/Typing_example.ogv) ;
- Start buffering, and wait for the download to complete (until all the file has buffered).

Expected:
- networkState is NETWORK_IDLE (1).

Actual result:
- networkState is NETWORK_LOADING (2). Additionnaly, a variable fraction of the buffering progress bar is not filled, despite having |buffered.end(0) == duration|.

That seems to indicate that we don't fire a "progress" event at the end of the download.
Comment 1 Paul Adenot (:padenot) 2012-06-06 16:52:55 PDT
Created attachment 630771 [details] [diff] [review]
Patch v0: special case the OGG seek.

This is probably because we download the end of the file to get the duration, when playing an OGG file, and this ensue :

- ChannelMediaResource::OnStopRequest is called, because the underlying channel is closed, because we hit the end of the resource, so the channels is closed without us suspending it ;
- mIgnoreClose is false, because it is never set to true ;
- Hence, nsMediaCacheStream::NotifyDataEnded is called, with mDidNotifyDataEnded to false, since it is the first time we call that method ;
- mDidNotifyDataEnded is flipped to true, which prevent subsequent call of this method to call ChannelMediaResource::CacheClientNotifyDataEnded, which prevent calling nsHTMLMediaElement::ResourceLoaded, and that does not change the networkState and does not fire the last progress, so the buffered bar does not update.

This patch special-cases when seeking to the end to get the duration. This kind of breaks the nice abstractions we have, though.
Comment 2 Paul Adenot (:padenot) 2012-06-21 14:52:58 PDT
Created attachment 635491 [details] [diff] [review]
Patch v1: don't forget to initialize the new member in the constructor.
Comment 3 Matthew Gregan [:kinetik] 2012-07-20 12:15:29 PDT
Sorry for the drive-by, but I saw this while looking at a related bug.

+  // If we are seeking to get the duration, because we are playing an OGG file,
+  // ignore if the channel gets closed without us suspending it explicitly. We
+  // don't want to tell the element that the download has finished.
+  if (mSeekingForDuration) {
+    mIgnoreClose = true;
+  } else {
+    mIgnoreClose = false;
+  }

This can just be:

  mIgnoreClose = mSeekingForDuration;

Can you also rename all the "ForDuration" things to "ForMetadata" and remove the specific mention of Ogg (since this may be used for other situations)? That matches better with some other places we've handled this sort of thing (c.f. calls to SetReadMode(MODE_METADATA)).
Comment 4 Paul Adenot (:padenot) 2012-07-20 12:26:59 PDT
Will do, thanks for the input!
Comment 5 Paul Adenot (:padenot) 2012-08-17 10:01:53 PDT
Created attachment 652803 [details] [diff] [review]
Don't notify the HTMLMediaElement if we reach the end of the media while seeking for metadata.

If forgot to re-upload this one. This addresses comment 3 remarks. Carrying
forward r+.
Comment 6 Daniel Holbert [:dholbert] 2012-08-17 12:08:38 PDT
Made one drive-by whitespace fix here:
>+void ChannelMediaResource::StartSeekingForMetadata()
>+{
>+  mSeekingForMetadata= true;
-----------------------^
and landed: https://hg.mozilla.org/integration/mozilla-inbound/rev/d7d6e90e2580
Comment 7 Ryan VanderMeulen [:RyanVM] 2012-08-17 19:22:25 PDT
https://hg.mozilla.org/mozilla-central/rev/d7d6e90e2580

Note You need to log in before you can comment on or make changes to this bug.