Last Comment Bug 730765 - Media cache shouldn't be used when loading from blob: urls
: Media cache shouldn't be used when loading from blob: urls
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Audio/Video (show other bugs)
: Trunk
: x86 Mac OS X
: P1 normal (vote)
: mozilla18
Assigned To: Paul Adenot (:padenot)
:
Mentors:
Depends on: 799234
Blocks: 787474 782460 784274 785909 785999
  Show dependency treegraph
 
Reported: 2012-02-27 00:07 PST by Jonas Sicking (:sicking)
Modified: 2012-10-09 03:16 PDT (History)
21 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
+


Attachments
Patch to fix (22.26 KB, patch)
2012-02-27 00:07 PST, Jonas Sicking (:sicking)
roc: review+
cbiesinger: review-
Details | Diff | Review
Media cache shouldn't be used when loading from blob: urls. r= (24.55 KB, patch)
2012-07-20 13:15 PDT, Paul Adenot (:padenot)
no flags Details | Diff | Review
Tests for nsISeekableStream when backed by a nsMultiplexInputStream. r= (5.69 KB, patch)
2012-07-20 13:22 PDT, Paul Adenot (:padenot)
no flags Details | Diff | Review
Media cache shouldn't be used when loading from blob: urls. r= (24.60 KB, patch)
2012-08-10 10:12 PDT, Paul Adenot (:padenot)
cbiesinger: review+
Details | Diff | Review
Tests for nsISeekableStream when backed by a nsMultiplexInputStream. r= (7.19 KB, patch)
2012-08-10 10:13 PDT, Paul Adenot (:padenot)
cbiesinger: review+
Details | Diff | Review
Media cache shouldn't be used when loading from blob: urls. (28.06 KB, patch)
2012-08-15 15:45 PDT, Paul Adenot (:padenot)
no flags Details | Diff | Review
Tests for nsISeekableStream when backed by a nsMultiplexInputStream. (6.39 KB, patch)
2012-08-15 15:47 PDT, Paul Adenot (:padenot)
no flags Details | Diff | Review
Media cache shouldn't be used when loading from blob: urls. (29.14 KB, patch)
2012-08-21 16:01 PDT, Paul Adenot (:padenot)
no flags Details | Diff | Review
Tests for nsISeekableStream when backed by a nsMultiplexInputStream. (6.39 KB, patch)
2012-08-21 16:01 PDT, Paul Adenot (:padenot)
no flags Details | Diff | Review
Remove commented out code that should have been removed. r= (906 bytes, patch)
2012-08-21 16:17 PDT, Paul Adenot (:padenot)
cbiesinger: review+
Details | Diff | Review
Added r=biesi (911 bytes, patch)
2012-08-21 17:42 PDT, Paul Adenot (:padenot)
padenot: review+
Details | Diff | Review
Hack around main thread IO (941 bytes, patch)
2012-08-27 08:48 PDT, Ben Turner (not reading bugmail, use the needinfo flag!)
no flags Details | Diff | Review
Media cache shouldn't be used when loading from blob: urls. (28.54 KB, patch)
2012-08-30 10:21 PDT, Paul Adenot (:padenot)
no flags Details | Diff | Review
Remove a copy of nsBlobProtocolHandler.h. r= (1.84 KB, patch)
2012-09-25 15:16 PDT, Paul Adenot (:padenot)
no flags Details | Diff | Review
Remove a copy of nsBlobProtocolHandler.h. (1.83 KB, patch)
2012-09-25 16:24 PDT, Paul Adenot (:padenot)
no flags Details | Diff | Review

Description Jonas Sicking (:sicking) 2012-02-27 00:07:18 PST
Created attachment 600852 [details] [diff] [review]
Patch to fix

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.
Comment 1 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-02-27 01:13:36 PST
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))
Comment 2 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-02-27 01:14:57 PST
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.)
Comment 3 Jonas Sicking (:sicking) 2012-02-27 02:07:57 PST
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 4 Boris Zbarsky [:bz] 2012-02-27 08:21:16 PST
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?
Comment 5 Boris Zbarsky [:bz] 2012-02-27 08:21:50 PST
For example, that would get us using a stream for data: URIs for free, I think.
Comment 6 Jonas Sicking (:sicking) 2012-02-27 08:32:44 PST
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 7 Christian :Biesinger (don't email me, ping me on IRC) 2012-02-27 13:12:07 PST
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.
Comment 8 Jonas Sicking (:sicking) 2012-02-27 15:25:27 PST
(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.
Comment 9 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-02-28 06:26:07 PST
(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.
Comment 10 David Flanagan [:djf] 2012-06-11 15:54:24 PDT
The B2G Music and Video apps read from blob urls derived from DeviceStorage files. I'd really love to get this fix landed.
Comment 11 Dietrich Ayala (:dietrich) 2012-06-11 16:18:36 PDT
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.
Comment 12 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-07-18 20:04:30 PDT
Chris P, please take this.
Comment 13 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-07-19 06:43:11 PDT
Or, maybe Paul? Or both of you together? We need this ASAP :-)
Comment 14 Jonas Sicking (:sicking) 2012-07-19 13:17:54 PDT
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.
Comment 15 Paul Adenot (:padenot) 2012-07-19 14:25:10 PDT
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.
Comment 16 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-07-19 20:54:16 PDT
Yes, he meant nsISeekableStream.
Comment 17 Paul Adenot (:padenot) 2012-07-20 13:15:25 PDT
Created attachment 644437 [details] [diff] [review]
Media cache shouldn't be used when loading from blob: urls. r=
Comment 18 Paul Adenot (:padenot) 2012-07-20 13:22:19 PDT
Created attachment 644442 [details] [diff] [review]
Tests for nsISeekableStream when backed by a nsMultiplexInputStream. r=

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.
Comment 19 Paul Adenot (:padenot) 2012-07-20 13:25:25 PDT
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.
Comment 20 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-08-01 16:24:28 PDT
Can we ask someone else for review here?
Comment 21 Chris Pearce (:cpearce) 2012-08-07 16:16:50 PDT
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?
Comment 22 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-08-09 19:58:36 PDT
Josh, see comment 21.
Comment 23 Jason Duell [:jduell] (needinfo? me) 2012-08-09 21:34:39 PDT
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.
Comment 24 Jason Duell [:jduell] (needinfo? me) 2012-08-09 21:36:43 PDT
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)
Comment 25 Jason Duell [:jduell] (needinfo? me) 2012-08-09 23:27:35 PDT
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?
Comment 26 Paul Adenot (:padenot) 2012-08-10 10:12:26 PDT
Created attachment 650922 [details] [diff] [review]
Media cache shouldn't be used when loading from blob: urls. r=

Here is the rebased patch. Tests are still passing.
Comment 27 Paul Adenot (:padenot) 2012-08-10 10:13:20 PDT
Created attachment 650923 [details] [diff] [review]
Tests for nsISeekableStream when backed by a nsMultiplexInputStream. r=

Uploading again, I spotted a typo while re-running the tests to make sure I
rebased correctly.
Comment 28 Jason Duell [:jduell] (needinfo? me) 2012-08-14 10:57:12 PDT
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...
Comment 29 Christian :Biesinger (don't email me, ping me on IRC) 2012-08-14 11:45:17 PDT
(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 30 Christian :Biesinger (don't email me, ping me on IRC) 2012-08-14 13:41:21 PDT
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
Comment 31 Christian :Biesinger (don't email me, ping me on IRC) 2012-08-14 13:44:11 PDT
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 :)
Comment 32 Paul Adenot (:padenot) 2012-08-14 13:46:33 PDT
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.
Comment 33 Paul Adenot (:padenot) 2012-08-15 15:45:59 PDT
Created attachment 652261 [details] [diff] [review]
Media cache shouldn't be used when loading from blob: urls.

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.
Comment 34 Paul Adenot (:padenot) 2012-08-15 15:47:01 PDT
Created attachment 652262 [details] [diff] [review]
Tests for nsISeekableStream when backed by a nsMultiplexInputStream.

This is the same tests, with a couple more assertions to reflect the code
changed to address the review comments.
Comment 35 Paul Adenot (:padenot) 2012-08-16 11:24:08 PDT
This is green on try: https://tbpl.mozilla.org/?tree=Try&rev=e1034e749b03
Comment 36 Jonas Sicking (:sicking) 2012-08-16 17:04:16 PDT
Yay Paul! Thanks for finishing this up!
Comment 37 Paul Adenot (:padenot) 2012-08-20 15:21:45 PDT
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
Comment 38 Paul Adenot (:padenot) 2012-08-21 11:42:04 PDT
This is perfectly green apart from what it seems to be infra problem on android, I guess we can land.
Comment 39 Ryan VanderMeulen [:RyanVM] 2012-08-21 15:25:46 PDT
(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
Comment 40 Ryan VanderMeulen [:RyanVM] 2012-08-21 15:46:54 PDT
...and backed out because I suck at unbitrotting. Please rebase and post new patches.
https://hg.mozilla.org/integration/mozilla-inbound/rev/492ef59eb268
Comment 41 Paul Adenot (:padenot) 2012-08-21 16:01:09 PDT
Created attachment 653993 [details] [diff] [review]
Media cache shouldn't be used when loading from blob: urls.

(rebased patch, ready for landing)
Comment 42 Paul Adenot (:padenot) 2012-08-21 16:01:53 PDT
Created attachment 653994 [details] [diff] [review]
Tests for nsISeekableStream when backed by a nsMultiplexInputStream.

(rebased patch)
Comment 44 Christian :Biesinger (don't email me, ping me on IRC) 2012-08-21 16:11:54 PDT
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?!
Comment 45 Paul Adenot (:padenot) 2012-08-21 16:13:59 PDT
Silly me. Those line should be removed. I'll write a patch right now.
Comment 46 Paul Adenot (:padenot) 2012-08-21 16:17:09 PDT
Created attachment 654000 [details] [diff] [review]
Remove commented out code that should have been removed. r=

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.
Comment 47 Paul Adenot (:padenot) 2012-08-21 17:42:07 PDT
Created attachment 654035 [details] [diff] [review]
Added r=biesi
Comment 50 cajbir (:cajbir) 2012-08-22 23:03:02 PDT
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.
Comment 51 cajbir (:cajbir) 2012-08-22 23:04:03 PDT
See bug 784274.
Comment 52 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-08-22 23:10:07 PDT
bent might be able to help out here.  Blob should Just Work when OOP.
Comment 53 Jonas Sicking (:sicking) 2012-08-22 23:56:48 PDT
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?
Comment 54 cajbir (:cajbir) 2012-08-23 01:09:03 PDT
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?
Comment 56 David Flanagan [:djf] 2012-08-23 06:58:32 PDT
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.
Comment 57 Andreas Gal :gal 2012-08-23 07:00:32 PDT
Chris, do you have an idea what was going wrong and when we can try landing this again?
Comment 58 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-08-23 10:47:01 PDT
(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.
Comment 60 Jonas Sicking (:sicking) 2012-08-23 19:58:06 PDT
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.
Comment 61 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-08-23 20:16:35 PDT
bent claims this is easy, if the problem here.
Comment 62 Ben Turner (not reading bugmail, use the needinfo flag!) 2012-08-24 15:45:51 PDT
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.
Comment 63 Ben Turner (not reading bugmail, use the needinfo flag!) 2012-08-27 08:32:20 PDT
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.
Comment 64 Ben Turner (not reading bugmail, use the needinfo flag!) 2012-08-27 08:48:49 PDT
Created attachment 655602 [details] [diff] [review]
Hack around main thread IO
Comment 65 Ben Turner (not reading bugmail, use the needinfo flag!) 2012-08-27 12:33:52 PDT
(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.
Comment 66 Chris Pearce (:cpearce) 2012-08-27 20:40:39 PDT
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...
Comment 67 Jonas Sicking (:sicking) 2012-08-27 21:18:41 PDT
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.
Comment 68 Chris Pearce (:cpearce) 2012-08-27 21:50:53 PDT
Yes. The Available() call would then be done off main thread when Seek/Tell/Tread/GetLength() was called.
Comment 69 Chris Pearce (:cpearce) 2012-08-27 22:41:40 PDT
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...
Comment 70 Chris Pearce (:cpearce) 2012-08-29 13:08:12 PDT
Bug 785909 has landed, can this now land again?
Comment 71 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-08-29 13:28:37 PDT
Is there a person around with a b2g env who can quickly hand tryserver these on latest?  /me in the middle of bisecting.
Comment 72 Paul Adenot (:padenot) 2012-08-29 13:30:05 PDT
I'll do that right now.
Comment 73 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-08-29 13:58:54 PDT
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.
Comment 74 Paul Adenot (:padenot) 2012-08-29 19:06:11 PDT
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.
Comment 75 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-08-29 19:07:10 PDT
\o/

Music works too?
Comment 76 Paul Adenot (:padenot) 2012-08-29 19:15:45 PDT
Ah, yes, forgot to mention that, the music app works as well.
Comment 77 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-08-29 19:17:31 PDT
\o/ \o/
Comment 78 Ben Turner (not reading bugmail, use the needinfo flag!) 2012-08-29 19:25:18 PDT
Ship it!
Comment 79 Paul Adenot (:padenot) 2012-08-30 10:21:59 PDT
Created attachment 656936 [details] [diff] [review]
Media cache shouldn't be used when loading from blob: urls.

Also, this needed rebasing over the PRInt -> int changes, plus another occurence
of the Count() -> Length() change.
Comment 80 Paul Adenot (:padenot) 2012-09-04 14:55:31 PDT
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
Comment 81 Chris Pearce (:cpearce) 2012-09-05 17:16:14 PDT
(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
Comment 83 Kyle Huey [:khuey] (khuey@mozilla.com) (Away until 6/13) 2012-09-25 15:05:22 PDT
There are now two copies of nsBlobProtocolHandler.h in the tree.  Please remove one of them.
Comment 84 Paul Adenot (:padenot) 2012-09-25 15:16:00 PDT
Created attachment 664668 [details] [diff] [review]
Remove a copy of nsBlobProtocolHandler.h. r=
Comment 85 Paul Adenot (:padenot) 2012-09-25 16:09:22 PDT
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.
Comment 86 Kyle Huey [:khuey] (khuey@mozilla.com) (Away until 6/13) 2012-09-25 16:14:38 PDT
Comment on attachment 664668 [details] [diff] [review]
Remove a copy of nsBlobProtocolHandler.h. r=

Just land it.
Comment 87 Paul Adenot (:padenot) 2012-09-25 16:24:00 PDT
Created attachment 664703 [details] [diff] [review]
Remove a copy of nsBlobProtocolHandler.h.
Comment 88 Paul Adenot (:padenot) 2012-09-25 16:26:07 PDT
https://bugzilla.mozilla.org/attachment.cgi?id=664703 is the patch that needs landing.
Comment 89 Ryan VanderMeulen [:RyanVM] 2012-09-26 15:57:41 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/20b1b4fd678e
Comment 90 Ed Morley [:emorley] 2012-09-27 04:01:08 PDT
https://hg.mozilla.org/mozilla-central/rev/20b1b4fd678e

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