The default bug view has changed. See this FAQ.

After having finished the download of an OGG video, networkState is NETWORK_LOADING.

RESOLVED FIXED in mozilla17

Status

()

Core
Audio/Video
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: padenot, Assigned: padenot)

Tracking

Trunk
mozilla17
x86_64
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

5 years ago
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.
(Assignee)

Comment 1

5 years ago
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.
Assignee: nobody → paul
Status: NEW → ASSIGNED
Attachment #630771 - Flags: review?(chris.double)
(Assignee)

Comment 2

5 years ago
Created attachment 635491 [details] [diff] [review]
Patch v1: don't forget to initialize the new member in the constructor.
Attachment #635491 - Flags: review?
(Assignee)

Updated

5 years ago
Attachment #635491 - Flags: review? → review?(chris.double)
(Assignee)

Updated

5 years ago
Attachment #630771 - Attachment is obsolete: true
Attachment #630771 - Flags: review?(chris.double)

Updated

5 years ago
Attachment #635491 - Flags: review?(chris.double) → review+
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)).
(Assignee)

Comment 4

5 years ago
Will do, thanks for the input!
(Assignee)

Comment 5

5 years ago
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+.
(Assignee)

Updated

5 years ago
Attachment #635491 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #652803 - Flags: review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
Made one drive-by whitespace fix here:
>+void ChannelMediaResource::StartSeekingForMetadata()
>+{
>+  mSeekingForMetadata= true;
-----------------------^
and landed: https://hg.mozilla.org/integration/mozilla-inbound/rev/d7d6e90e2580
Keywords: checkin-needed
Version: unspecified → Trunk
https://hg.mozilla.org/mozilla-central/rev/d7d6e90e2580
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
You need to log in before you can comment on or make changes to this bug.