Closed Bug 730765 Opened 12 years ago Closed 12 years ago

Media cache shouldn't be used when loading from blob: urls

Categories

(Core :: Audio/Video, defect, P1)

x86
macOS
defect

Tracking

()

RESOLVED FIXED
mozilla18
blocking-kilimanjaro +
blocking-basecamp +

People

(Reporter: sicking, Assigned: padenot)

References

Details

Attachments

(4 files, 11 obsolete files)

6.39 KB, patch
Details | Diff | Splinter Review
911 bytes, patch
padenot
: review+
Details | Diff | Splinter Review
28.54 KB, patch
Details | Diff | Splinter Review
1.83 KB, patch
Details | Diff | Splinter Review
Attached patch Patch to fix (obsolete) — Splinter Review
When loading from a file url we avoid caching the loaded data in the media cache since it's just as fast to load directly from the resource. However when loading from a blob: URL we don't realize that the same optimization can be done which means wasting time copying file data.

Patch coming up to fix this. The majority of the patch makes sure that nsMultiplexInputStream properly implements nsISeekableStream since some blob: URLs are backed by such a stream.

Requesting review from roc for the mediacache parts, though feel free to forward the review.

Requesting biesi's sr for the xpcom and netwerk changes.
Attachment #600852 - Flags: review?(roc)
Comment on attachment 600852 [details] [diff] [review]
Patch to fix

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

r+ on the MediaResource changes

::: content/media/MediaResource.cpp
@@ +1024,5 @@
> +      NS_ENSURE_SUCCESS(rv, rv);
> +  
> +      rv = NS_NewLocalFileInputStream(getter_AddRefs(mInput), file);
> +    }
> +    else if (IsBlobURI(mURI)) {

} else if (...) {

@@ +1188,5 @@
>    NS_ENSURE_SUCCESS(rv, nsnull);
>  
>    nsCOMPtr<nsIFileChannel> fc = do_QueryInterface(aChannel);
>    if (fc) {
>      return new FileMediaResource(aDecoder, aChannel, uri);

if (fc || IsBlobURI(uri))
Attachment #600852 - Flags: review?(roc) → review+
Oh, just one question though: is it really safe to access that blob stream from multiple threads? (Note, all accesses to the stream are guarded by a mutex in FileMediaReource.)
As long as you don't read from several threads at the same time, which seems inherently unsafe.

Blob-streams are always backed by file streams, memory streams or multiplex streams.

We already access the streams from multiple threads in that when you submit a blob using XHR they are created on the main thread, but read on the socket thread.
Comment on attachment 600852 [details] [diff] [review]
Patch to fix

A question about the media cache side... Can that just test URI_IS_LOCAL_RESOURCE and then use Open() on a channel instead of using ad-hoc methods for getting a stream based on protocol?
For example, that would get us using a stream for data: URIs for free, I think.
For something like

<video id=v></video>
<script>
var url = getURLSomehow();
document.getElementById('v').src = url;
</script>

We seem to have called AsyncOpen on the channel before we get to the media cache.

Not sure if that's something that can be changed though?

And for something like typing a URL in the URL-bar we've definitely called AsyncOpen on the channel already. Of course, it's unlikely that that will happen commonly with data: url or blob: urls.
Comment on attachment 600852 [details] [diff] [review]
Patch to fix

+++ b/content/base/src/nsBlobProtocolHandler.cpp
+  nsCOMPtr<nsIInputStream> stream;
+  nsresult rv = info->mFile->GetInternalStream(getter_AddRefs(stream));
+  NS_ENSURE_SUCCESS(rv, rv);
+
+  stream.forget(aStream);
+  
+  return NS_OK;

Those lines can be replaced with:
  return info->mFile->GetInternalStream(aStream);
right?

+++ b/netwerk/base/src/nsFileStreams.cpp

I don't understand why you need mCachedPosition. REOPEN_ON_REWIND means the position should then be zero, right?

+++ b/xpcom/io/nsMultiplexInputStream.cpp
+    if (aWhence == NS_SEEK_SET) {

Have you considered converting SEEK_CUR/SEEK_END to SEEK_SET by adjusting aOffset using Tell()/sum of Available()?

+            nsCOMPtr<nsISeekableStream> stream =
+                do_QueryInterface(mStreams[i]);

You need a nullcheck, right?

+            if (remaining == 0) {
+                if (i < oldCurrentStream ||
+                    (i == oldCurrentStream && oldStartedReadingCurrent)) {
+                    rv = stream->Seek(NS_SEEK_SET, 0);
+                    NS_ENSURE_SUCCESS(rv, rv);

Need a continue here?

+            // Get possition in current stream

typo: possition -> position




Also, Seek really needs a test, the function is now fairly complex.
Attachment #600852 - Flags: review?(cbiesinger) → review-
(In reply to Christian :Biesinger (don't email me, ping me on IRC) from comment #7)
> Those lines can be replaced with:
>   return info->mFile->GetInternalStream(aStream);
> right?

Yes. Will fix.

> +++ b/netwerk/base/src/nsFileStreams.cpp
> 
> I don't understand why you need mCachedPosition. REOPEN_ON_REWIND means the
> position should then be zero, right?

The way the multiplex stream works is that it reads each stream until its end and then starts reading the next stream. That means that streams that CLOSE_ON_EOF file-streams automatically close once we move on to the next stream.

If there's a Seek(SET) call after we're done consuming a few of the streams (or all of them) that means that we call Tell on closed streams since the multiplex channel calls Tell to figure out which sub-streams to rewind and which to leave as-is. This Tell call will fail if the underlying stream was automatically closed.

That makes me realize, I need to add a similar codepath to Available() to just return 0 in the same situation.

> +++ b/xpcom/io/nsMultiplexInputStream.cpp
> +    if (aWhence == NS_SEEK_SET) {
> 
> Have you considered converting SEEK_CUR/SEEK_END to SEEK_SET by adjusting
> aOffset using Tell()/sum of Available()?

Wouldn't that be slower? I assume that at least Available() requires file IO to happen. I'm not sure if that's true for Tell too since it needs to guard against the file getting truncated past the point that we've read.

Basically I've tried to optimize for few calls to Seek/Tell/Available.

> +            nsCOMPtr<nsISeekableStream> stream =
> +                do_QueryInterface(mStreams[i]);
> 
> You need a nullcheck, right?

Yes, I lost that somewhere along the way.

> +            if (remaining == 0) {
> +                if (i < oldCurrentStream ||
> +                    (i == oldCurrentStream && oldStartedReadingCurrent)) {
> +                    rv = stream->Seek(NS_SEEK_SET, 0);
> +                    NS_ENSURE_SUCCESS(rv, rv);
> 
> Need a continue here?

Good catch! Might not be strictly needed since it passed my tests, but it's definitely the right thing to do.

> Also, Seek really needs a test, the function is now fairly complex.

Agreed.
(In reply to Boris Zbarsky (:bz) from comment #4)
> A question about the media cache side... Can that just test
> URI_IS_LOCAL_RESOURCE and then use Open() on a channel instead of using
> ad-hoc methods for getting a stream based on protocol?

We probably could alter nsHTMLMediaElement to do that, but having a sync vs async path seems unnecessarily complex for the benefit gained. I don't think data: URIs for large media resources is an important use-case.
The B2G Music and Video apps read from blob urls derived from DeviceStorage files. I'd really love to get this fix landed.
I'm going to assert that if we're shipping video in V1 of B2G phone, that seeking in that video is a requirement, so marking blocking. We can revisit at triage, if others disagree.
blocking-basecamp: --- → +
blocking-kilimanjaro: --- → +
Chris P, please take this.
Assignee: nobody → cpearce
Or, maybe Paul? Or both of you together? We need this ASAP :-)
Assignee: cpearce → paul
Just so that everyone is on the same page:

All that we need to do here is write tests for the new-and-improved support for nsISeekableChannel in the multiplex-channel.

Probably some type of xpcshell test would do.

Also address review comments, but those were very minor.
Jonas, did you meant "the new-and-improved support for nsISeekableStream when backed by a nsIMultiplexInputStream?". I can't seem to find an nsISeekableChannel anywhere.
Yes, he meant nsISeekableStream.
Here is an xpcshell test to ensure seeking works properly on
nsMultiplexInputStream.

This caught a couple bugs in the |nsMultiplexInputStream::Seek| implementation,
that are fixed, now.

Also I'm not sure when to throw. The documentation says that we should throw
only when the underlying stream is closed. The current implementation throws
when it gets passed a positive offset and we are seeking using NS_SEEK_END, but
this easily fixable if we decide it is not the right thing to do.

When passing an invalid offset and using NS_SEEK_SET, the value is clamped, but
the function does not throw. Again, this is fixable.
Attachment #644442 - Flags: review?(cbiesinger)
Attachment #600852 - Attachment is obsolete: true
Comment on attachment 644437 [details] [diff] [review]
Media cache shouldn't be used when loading from blob: urls. r=

This patch is rebased on current tip, and the review comments are addressed. A couple bug fixes are added in ::Seek, found whil writing the tests.
Attachment #644437 - Flags: review?(cbiesinger)
Can we ask someone else for review here?
jduell: we need this fix to ensure that videos can seek properly on B2G, and cbiesinger seems to be unavailable for review, would you be able to take a look at the patches here instead? Or can you suggest someone who'd be appropriate?
Priority: -- → P1
Comment on attachment 644442 [details] [diff] [review]
Tests for nsISeekableStream when backed by a nsMultiplexInputStream. r=

biesi doesn't seem to be coming back to IRC any time soon--taking.
Attachment #644442 - Flags: review?(cbiesinger) → review?(jduell.mcbugs)
Comment on attachment 644437 [details] [diff] [review]
Media cache shouldn't be used when loading from blob: urls. r=

I'll look over everything, but I assume I'll need some sort of signoff for the bits in /content/base and the tests in /xpcom?  (roc has already r'd the media bits)
Attachment #644437 - Flags: review?(cbiesinger) → review?(jduell.mcbugs)
Comment on attachment 644437 [details] [diff] [review]
Media cache shouldn't be used when loading from blob: urls. r=

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

Paul,  

If you get a chance to rebase this patch (it's not applying cleanly: see below) that'd be great.

::: netwerk/base/src/nsFileStreams.h
@@ -113,5 @@
>      NS_IMETHOD Close();
> -    NS_IMETHOD Available(PRUint32* _retval)
> -    {
> -        return nsFileStreamBase::Available(_retval);
> -    }

I don't see any recent version of this file in hg that has this inline version of Available that you're treating as the existing version.  Are you basing this on top of another patch?

@@ +125,5 @@
>      } 
>      
>      // Overrided from nsFileStreamBase
>      NS_IMETHOD Seek(PRInt32 aWhence, PRInt64 aOffset);
> +    NS_IMETHOD Tell(PRInt64 *aResult);

Also, Seek() was removed from this file (it's now implied by nsISeekableStream) back in May.   Can you check that none of your changes conflict with bug 757507?
Here is the rebased patch. Tests are still passing.
Attachment #650922 - Flags: review?(jduell.mcbugs)
Attachment #644437 - Attachment is obsolete: true
Attachment #644437 - Flags: review?(jduell.mcbugs)
Uploading again, I spotted a typo while re-running the tests to make sure I
rebased correctly.
Attachment #650923 - Flags: review?(jduell.mcbugs)
Attachment #644442 - Attachment is obsolete: true
Attachment #644442 - Flags: review?(jduell.mcbugs)
Comment on attachment 650922 [details] [diff] [review]
Media cache shouldn't be used when loading from blob: urls. r=

biesi's reviewing these as I type.  Sorry for delay...
Attachment #650922 - Flags: review?(jduell.mcbugs) → review?(cbiesinger)
Attachment #650923 - Flags: review?(jduell.mcbugs) → review?(cbiesinger)
(In reply to Jonas Sicking (:sicking) from comment #8)
> > Have you considered converting SEEK_CUR/SEEK_END to SEEK_SET by adjusting
> > aOffset using Tell()/sum of Available()?
> 
> Wouldn't that be slower? I assume that at least Available() requires file IO
> to happen. I'm not sure if that's true for Tell too since it needs to guard
> against the file getting truncated past the point that we've read.
> 
> Basically I've tried to optimize for few calls to Seek/Tell/Available.

Hm OK. I sort of think that in this case, the speed benefit is not worth the added code complexity but I'll let you do it either way.
Comment on attachment 650922 [details] [diff] [review]
Media cache shouldn't be used when loading from blob: urls. r=

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

I guess I'll give this a r+, but there's some important notes below.

::: content/media/MediaResource.cpp
@@ +1020,5 @@
>      // The channel is already open. We need a synchronous stream that
>      // implements nsISeekableStream, so we have to find the underlying
>      // file and reopen it
>      nsCOMPtr<nsIFileChannel> fc(do_QueryInterface(mChannel));
> +    if (fc || IsBlobURI(uri)) {

This change can't be right. If fc is null but IsBlobUri is true, you'll crash two lines down. Just remove that part of the if?

(is this missing a test?)

@@ +1024,5 @@
> +    if (fc || IsBlobURI(uri)) {
> +      nsCOMPtr<nsIFile> file;
> +      rv = fc->GetFile(getter_AddRefs(file));
> +      NS_ENSURE_SUCCESS(rv, rv);
> +  

no trailing whitespace, please

::: netwerk/base/src/nsFileStreams.cpp
@@ +461,5 @@
> +            NS_ENSURE_SUCCESS(rv, rv);
> +
> +            if (aWhence == NS_SEEK_CUR) {
> +                aWhence = NS_SEEK_SET;
> +                aOffset += mCachedPosition;

So now that this class supports seeks other than Seek(0, SEEK_SET) with REOPEN_ON_REWIND, it seems like you should update the comments in http://mxr.mozilla.org/mozilla-central/source/netwerk/base/public/nsIFileStreams.idl#47

@@ +486,5 @@
> +NS_IMETHODIMP
> +nsFileInputStream::Available(PRUint32 *aResult)
> +{
> +    if (mFD == nullptr && mBehaviorFlags & REOPEN_ON_REWIND) {
> +        *aResult = 0;

this behaviour is a little weird and arguably wrong when the file was closed when it wasn't at EOF, consider:

  // stream is a file of size 10
  stream->Seek(0, SEEK_SET);
  stream->Read(5);
  stream->Close();
  // Now stream->Tell() will be 5 but Available() will be 0, does that make any sense?


I also feel that there should be comments in the places that you're modifying in this file explaining the thinking more clearly.

::: xpcom/io/nsMultiplexInputStream.cpp
@@ +96,5 @@
>  /* void insertStream (in nsIInputStream stream, in unsigned long index); */
>  NS_IMETHODIMP
>  nsMultiplexInputStream::InsertStream(nsIInputStream *aStream, PRUint32 aIndex)
>  {
> +    mStreams.InsertElementAt(aIndex, aStream);

Shouldn't you keep the OOM check?

@@ +365,5 @@
> +            // See if we need to seek current stream forward or backward
> +            if (remaining < streamPos) {
> +                rv = stream->Seek(NS_SEEK_SET, remaining);
> +                NS_ENSURE_SUCCESS(rv, rv);
> +                

please don't add trailing whitespace
Attachment #650922 - Flags: review?(cbiesinger) → review+
Comment on attachment 650923 [details] [diff] [review]
Tests for nsISeekableStream when backed by a nsMultiplexInputStream. r=

-    if (fc || IsBlobURI(uri)) {
+    if (fc) {

heh, guess that answers my comment above. I didn't expect code changes in a "tests" patch :)
Attachment #650923 - Flags: review?(cbiesinger) → review+
Ho, sorry about putting code change in the tests, I must have qref'ed on the wrong patch.

I'll address the comments shortly, thanks for the review.
This patch addresses review comment, and fixes an obvious error when seeking
back to the beggining of a file.

I pushed to try, seems green so far.
Attachment #650922 - Attachment is obsolete: true
This is the same tests, with a couple more assertions to reflect the code
changed to address the review comments.
Attachment #650923 - Attachment is obsolete: true
Yay Paul! Thanks for finishing this up!
I've ran into a crash right after rebasing to land, so I've pushed to try to make sure: https://tbpl.mozilla.org/?tree=Try&rev=2845e46c3ee8
This is perfectly green apart from what it seems to be infra problem on android, I guess we can land.
Keywords: checkin-needed
(In reply to Paul ADENOT (:padenot) from comment #37)
> I've ran into a crash right after rebasing to land, so I've pushed to try to
> make sure: https://tbpl.mozilla.org/?tree=Try&rev=2845e46c3ee8

Green on Try.

https://hg.mozilla.org/integration/mozilla-inbound/rev/3ce94808577b
https://hg.mozilla.org/integration/mozilla-inbound/rev/56f288230d5a
Flags: in-testsuite+
Keywords: checkin-needed
...and backed out because I suck at unbitrotting. Please rebase and post new patches.
https://hg.mozilla.org/integration/mozilla-inbound/rev/492ef59eb268
Flags: in-testsuite+
(rebased patch, ready for landing)
Attachment #652261 - Attachment is obsolete: true
Attachment #652262 - Attachment is obsolete: true
Comment on attachment 653993 [details] [diff] [review]
Media cache shouldn't be used when loading from blob: urls.

+nsFileInputStream::Available(PRUint64 *aResult)
+{
+    //if (mFD == nullptr && mBehaviorFlags & REOPEN_ON_REWIND) {
+        //*aResult = 0;
+        //return NS_OK;
+    //}

that doesn't look right?!
Silly me. Those line should be removed. I'll write a patch right now.
Per comment 30, I commented out those lines to check that they were the cause of
problems on try, but I forgot to remove them entirely.
Attachment #654000 - Flags: review?(cbiesinger)
Attachment #654000 - Flags: review?(cbiesinger) → review+
Attached patch Added r=biesiSplinter Review
Attachment #654000 - Attachment is obsolete: true
Attachment #654035 - Flags: review+
Just a heads up that we plan to back out these patches due to a b2g issue. With these patches applied the music player does not work. Attempting to play a file results in no playback.

If the music player is blacklisted so that it doesn't run out of process then it does work with these patches. So it is only the OOP case where this patch fails.
Blocks: 784274
bent might be able to help out here.  Blob should Just Work when OOP.
Bent: do we have tests for loading from blob:... using an OOP blob? I think we have tests that uses FileReader with an OOP blob, but I'm not sure if we have tests for for example doing an XHR-load from an OOP blob url, i.e. something like

xhr = new XMLHttpRequest(...);
xhr.open("GET", URL.createObjectURL(blob));
xhr.send();

Of course, I think it should "Just Work" given how the blob protocol scheme is implemented. And the fact that loading from blob: URLs when using the media cache seems to indicate that it does.


Ooh, I wonder if the issue is that the nsISeekableStream implementation of a OOP blob stream doesn't work as needed?
I've been unable to back this out due to the number of merge errors as a result of changes since it landed and I'm not familiar enough with the code to be confident to fix it myself. Paul, can you back this out please?
Darn.  I wish we could have left this patch in and moved the Gaia media apps back on process while awaiting a fix for this.

I've been waiting weeks and weeks for this to land in hopes that it would make videos loaded from blob: URLs seekable.  Without a fix for this bug, not much progress is possible on the Video app.
Chris, do you have an idea what was going wrong and when we can try landing this again?
(In reply to David Flanagan [:djf] from comment #56)
> Darn.  I wish we could have left this patch in and moved the Gaia media apps
> back on process while awaiting a fix for this.
> 

They're not OOP.

A/V from Blob worked OOP, then this bug regressed that.  This bug is blocking moving music/video apps OOP.
Target Milestone: mozilla17 → ---
My best guess is that seeking in OOP streams never worked. We just didn't do it before since the media cache was used go implement seeking. With this patch we use the full seeking API on OOP blob streams.
bent claims this is easy, if the problem here.
The code in comment 53 works. I'll add it to the Blob tests soon.

The problem has to be elsewhere. I'll keep digging.
Ok, I found the problem. OOP Blob input streams do not support blocking IO on the main thread, and MediaResource calls Available on the main thread:

http://mxr.mozilla.org/mozilla-central/source/content/media/MediaResource.cpp#1078

I can hack around this by making a failing Available call set mSize to -1. That makes everything work. However, we need to fix this main thread IO.
(In reply to ben turner [:bent] from comment #62)
> The code in comment 53 works. I'll add it to the Blob tests soon.

In bug 786003.
Blocks: 785999
Comment on attachment 655602 [details] [diff] [review]
Hack around main thread IO

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

I think we should instead check to see if FileMediaResource::mSize has been initialized inside FileMediaResource::Seek,Tell,Read, and GetLength(), and if not initialize it there (and dispatch LoadedEvent there too). Those function shouldn't be called on the main thread, so this should be OK. This way our behaviour shouldn't change as much...
Chris: If we do that, can we also remove the Available() call from the main thread completely? That way we'd avoid sync IO in Firefox.
Yes. The Available() call would then be done off main thread when Seek/Tell/Tread/GetLength() was called.
There's a patch over in Bug 785909 which implements my suggested fix for the main thread I/O in FileMediaResource::Open(). It would be good if someone could test it...
Bug 785909 has landed, can this now land again?
Is there a person around with a b2g env who can quickly hand tryserver these on latest?  /me in the middle of bisecting.
I'll do that right now.
Thanks!

You'll want to manually move the 'Video' app out of the OOP blacklist in $gaia/apps/system/js/window_manager.js, because a fix it depends is still only landed on inbound.
Chris, following your instructions (commenting the line that blacklists the Video app), and with a b2g build with this patch, I can play a video in the video app.
Ah, yes, forgot to mention that, the music app works as well.
Also, this needed rebasing over the PRInt -> int changes, plus another occurence
of the Count() -> Length() change.
Attachment #653993 - Attachment is obsolete: true
Attachment #655602 - Attachment is obsolete: true
Attachment #655602 - Flags: review?(cpearce)
Alright, pushed to try at [1], works on device and on desktop, as far as I tested.

[1]: https://tbpl.mozilla.org/?tree=Try&rev=b2bef49edadb
(In reply to Paul ADENOT (:padenot) from comment #80)
> Alright, pushed to try at [1], works on device and on desktop, as far as I
> tested.
> 
> [1]: https://tbpl.mozilla.org/?tree=Try&rev=b2bef49edadb

This looks green, I landed those changesets on inbound:

https://hg.mozilla.org/integration/mozilla-inbound/rev/e31c400df7a1
https://hg.mozilla.org/integration/mozilla-inbound/rev/d510742da8e9
https://hg.mozilla.org/mozilla-central/rev/e31c400df7a1
https://hg.mozilla.org/mozilla-central/rev/d510742da8e9
Status: REOPENED → RESOLVED
Closed: 12 years ago12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
There are now two copies of nsBlobProtocolHandler.h in the tree.  Please remove one of them.
Comment on attachment 664668 [details] [diff] [review]
Remove a copy of nsBlobProtocolHandler.h. r=

I kept the version in public/, and checked that they are identical.
Attachment #664668 - Flags: review?(khuey)
Comment on attachment 664668 [details] [diff] [review]
Remove a copy of nsBlobProtocolHandler.h. r=

Just land it.
Attachment #664668 - Flags: review?(khuey)
Attachment #664668 - Attachment is obsolete: true
https://bugzilla.mozilla.org/attachment.cgi?id=664703 is the patch that needs landing.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/20b1b4fd678e
Status: REOPENED → RESOLVED
Closed: 12 years ago12 years ago
Resolution: --- → FIXED
Depends on: 799234
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: