216218 - streaming sound will pause after a few seconds

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Christine Zimmerman]

User-Agent:       Mozilla/4.0 (compatible; MSIE 6.0; Windows NT 5.1)
Build Identifier: 

playing streaming sound in Shockwave movie will fail after ~53% streamed

Reproducible: Always

Steps to Reproduce:
1. play SW movie that contains streaming SWA
2. 
3.

Actual Results:  
it will stop playing after ~53 % streamed audio

Expected Results:  
should play all 100%

Comment 1

[Matthias Versen [:Matti]]

Do you have an excample URL and which flash version Do you used ?

Comment 2

[Matthias Versen [:Matti]]

Attachment: testcase from the reporter

Comment 3

[Matthias Versen [:Matti]]

confirming with win2k build 20030814 and
Macromedia Shockwave for Director Netscape plug-in, version 8.5

The shockwave streaming stops at ~51% with a networking error.
The testcase works with the same plugin in NS4.7x

Comment 4

[[e-mail] (away - not reading bugmail)]

From Macromedia's public beta site:

http://www.macromedia.com/software/shockwaveplayer/beta/release_notes/

Macromedia will withhold Shockwave support for any Mozilla based browsers until 
the following bugs are fixed:

213331 QT spills and jumps all over Netscape 7.1 (OSX)
213329 Japanese error message doesn't display double-byte text correctly
216216 Editable text field does not allow text entry
216218 Netscape 7.x: Streaming sound issues

Comment 5

[chris hofmann]

nominate for 1.7 to see if we can get it in.

Comment 6

[Matthias Versen [:Matti]]

The testcase doesn't work at the moment because http://audio.macromedia.com
seems to be dead

Comment 7

[David Lawhon]

One example of this problem occurs at http://www.morganic.com/WardM/Music.html

For the first song (length 2:28), it stops playing for me at 13 seconds.
For the last song (length 3:44), it stops playing for me at 23 seconds.
So, for each one, it gets about 10% through before stopping.

I'm using the 3/30/04 OSX build of Mozilla, with Shockwave 10.0.0.210, under
MacOSX 10.3.3. (It plays completely in Safari 1.2.1 with the same version of
Shockwave.)

Comment 8

[Johnny Stenback (:jst)]

Attachment: Proposed fix. Suspend necko & manually pump data, then resume...

This patch is not exactly trivial, but it seems to work and I don't see how
this can be fixed w/o something along these lines. The problem we're running
into here is that we're getting data off the network faster than the plugin can
consume it, and we simply bail and close the stream etc etc. The fix is to not
bail, but to suspend the request, and spoonfeed the plugin with what data we
already got off the network, until it consumes most of it, then resume the
request to get more data off the network, and do that as long as there's more
data...

Comment 9

[Johnny Stenback (:jst)]

Attachment: diff -w of the above for review.

Comment 10

[Johnny Stenback (:jst)]

Attachment: Updated diff -w, fixes some plugin stream offset problems in the earlier version

Comment 11

[Johnny Stenback (:jst)]

Attachment: Updated diff -w, fixes some plugin stream offset problems in the earlier version, and also makes sure a suspended request is canceld when the plugin stream is destroyed.

Comment 12

[Darin Fisher]

Comment on attachment 145576 [details] [diff] [review]
Updated diff -w, fixes some plugin stream offset problems in the earlier version, and also makes sure a suspended request is canceld when the plugin stream is destroyed.

>Index: modules/plugin/base/src/ns4xPluginInstance.cpp

> ns4xPluginStreamListener::OnDataAvailable(nsIPluginStreamInfo* pluginInfo,
...
>       PRUint32 contentLength;
>       pluginInfo->GetLength(&contentLength);
>+
>+    mStreamBufferSize = PR_MAX(length, contentLength);

So, I don't think there's any point to setting mStreamBufferSize above
since its value is going to be changed immediately to something else
below:

>+
>+    // Limit the size of the initial buffer to MAX_PLUGIN_NECKO_BUFFER
>+    // (16k). This buffer will grow if needed, as in the case where
>+    // we're getting data faster than the plugin can process it.
>+    mStreamBufferSize = PR_MIN(length, MAX_PLUGIN_NECKO_BUFFER);


>+      if (amountRead == 0) {
>+        NS_WARNING("input->Read() returns no data, it's almost impossible "
>+                   "to get here");

Make this a NS_NOTREACHED instead since yeah, you are right.  This should
never happen.


>+class nsI4xPluginStreamInfo : public nsISupports

perhaps it makes sense for this interface to inherit from nsIPluginStreamInfo?


otherwise, this patch looks great to me.  sr=darin

Comment 13

[Johnny Stenback (:jst)]

The idea with the code that does the following:

    mStreamBufferSize = PR_MAX(length, contentLength);
    ...
    mStreamBufferSize = PR_MIN(length, MAX_PLUGIN_NECKO_BUFFER);

was to make us malloc a buffer large enough to fit the whole stream (i.e.
contentLength) but only if that's smaller than MAX_PLUGIN_NECKO_BUFFER, which
means that the code is incorrect, what I meant for it to do was this:

    mStreamBufferSize = PR_MAX(length, contentLength);
    ...
    mStreamBufferSize = PR_MIN(mStreamBufferSize, MAX_PLUGIN_NECKO_BUFFER);

since I don't want us to get into a situation where the first ODA call hands us
8 bytes (or whatever small number), and subsequent calls hands us 2kb, or
whatever, and in the end, the whole plugin stream would've fit in
MAX_PLUGIN_NECKO_BUFFER. This is trying to avoid us reading only a tiny bit of
data and then writing that to the plugin, and then reading a tiny bit again, and
so on.

I can go either way on the interface inheritance, I'll change that since you
pointed it out...

Comment 14

[Johnny Stenback (:jst)]

Attachment: Fix darins issues.

Comment 15

[Brendan Eich [:brendan]]

Comment on attachment 145641 [details] [diff] [review]
Fix darins issues.


>+  nsresult rv = NS_OK;
>+  while (NS_SUCCEEDED(rv) && length > 0) {
>+    if (input && length) {
>+      if (mStreamBufferSize < mStreamBufferByteCount + length &&
>+          mIsSuspended) {
>+        // We're in the ::OnDataAvailable() call that we might get
>+        // after suspending a request, or we suspended the request
>+        // from within this ::OnDataAvailable() call while there's
>+        // still data in the input, and we don't have enough space to
>+        // store what we got off the network. Reallocate our internal
>+        // buffer.
>+        mStreamBufferSize = mStreamBufferByteCount + length;
>+        mStreamBuffer = (char*)PR_Realloc(mStreamBuffer, mStreamBufferSize);

Use a temporary for the return value, so you don't leak the unrealloced (after
realloc failure) existing allocation, setting mStreamBuffer non-null only on
success.

>+        if (!mStreamBuffer)
>+          return NS_ERROR_OUT_OF_MEMORY;
>+      }
>-        if (numtowrite > amountRead)
>-          numtowrite = amountRead;
>+
>+          // Break out of the for loop, but keep going through the
>+          // while loop in case there's more data to read from the
>+          // input stream.

No more for loop, it's an outer while now, eh?

>+      // XXX: Why couldn't this be tracked on the plugin
>+      // info, and not in a *hash table*?
>+      nsPRUintKey key(absoluteOffset);
>+      PRInt32 amtForwardToPlugin =
>+        NS_PTR_TO_INT32(mDataForwardToRequest->Get(&key));

Can you file bug(s) as needed to followup on the XXX's you've added?  Maybe
these should be FIXME: <bug#> comments instead -- that's something the GNOME
guys do, and it tends to lead to further fixing better than ye olde XXX
comments.

sr=me for 1.7.	Thanks for wading into the swamp a bit!

/be

Comment 16

[Johnny Stenback (:jst)]

Attachment: Fix brendan's issues.

Comment 17

[Johnny Stenback (:jst)]

Comment on attachment 145785 [details] [diff] [review]
Fix brendan's issues.

Carrying r=darin and sr=brendan forward, and requesting approval for 1.7 finald
for this macromedia blocker.

Comment 18

[Asa Dotzler [:asa]]

Comment on attachment 145785 [details] [diff] [review]
Fix brendan's issues.

a=asa (on behalf of drivers) for checkin to 1.7

Comment 19

[Johnny Stenback (:jst)]

Fixed, but I incorrectly quoted bug 237076 in the checkin comment (looked at the
wrong window while typing), duh.

Marking bug 89270 fixed too, as it was an earlier report of the same problem
(AFAICT), IOW, this should've been a dupe of that bug.

Comment 20

[zug_treno]

Could somebody take a look at bug 241431 and bug 241592? They might be related
to this checkin.

Comment 21

[Johnny Stenback (:jst)]

Looking. Thanks.