Last Comment Bug 686137 - Setting audio.mozFrameBufferLength has no effect on MozAudioAvailable.frameBuffer.length
: Setting audio.mozFrameBufferLength has no effect on MozAudioAvailable.frameBu...
Status: RESOLVED FIXED
: regression
Product: Core
Classification: Components
Component: Audio/Video (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla12
Assigned To: Chris DeCairos (:cade)
:
Mentors:
Depends on: 638807
Blocks:
  Show dependency treegraph
 
Reported: 2011-09-10 10:11 PDT by David Humphrey (:humph)
Modified: 2012-01-25 07:14 PST (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Added virtual keyword to RequesteFramBufferLength (1.10 KB, patch)
2012-01-23 15:43 PST, Chris DeCairos (:cade)
kinetik: review+
Details | Diff | Review
Created a test for mozFrameBufferLength changes (2.28 KB, patch)
2012-01-23 19:35 PST, Chris DeCairos (:cade)
no flags Details | Diff | Review
Fixes for tests as per review (2.11 KB, patch)
2012-01-23 21:13 PST, Chris DeCairos (:cade)
kinetik: review+
Details | Diff | Review
Make nsMediaDecoder::RequestFrameBufferLength virtual (1.12 KB, patch)
2012-01-24 08:27 PST, Chris DeCairos (:cade)
no flags Details | Diff | Review

Description David Humphrey (:humph) 2011-09-10 10:11:48 PDT
Setting an audio's mozFrameBufferLength in loadedmetadata should cause the internal buffer size used for MozAudioAvailable events to be changed, as long as it's between 512 and 16384.  In FF4 this worked.  In FF6 and Nightly, assigning the value only changes the audio element's property, but doesn't push it down to the internal buffer logic.

Here's a simple test case:

    <audio id="input" src="corban-peddle.ogg" controls="true" onloadedmetadata="loadedMetaData(event);"></audio>
    <script>
      var audio,
          frameBufferSize = 4096;

      function loadedMetaData(event) {
	audio = document.getElementById('input');

      	// Default size	will be	Channels * 1024
	audio.mozFrameBufferLength = frameBufferSize;
	audio.addEventListener("MozAudioAvailable", audioAvailable, false);
      }

      function audioAvailable(event) {
	console.log(audio.mozFrameBufferLength, event.frameBuffer.length);
      } 
    </script>

This should print "4096, 4096" but it now prints "4096, 2048"
Comment 1 Chris DeCairos (:cade) 2012-01-12 09:41:03 PST
I can look into fixing this :D
Comment 2 Chris DeCairos (:cade) 2012-01-19 20:13:53 PST
I did some testing against humphs' tests.
mozAudioAvailable was reporting the correct length on April 11, 2011 (https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2011/04/2011-04-11-03-mozilla-central/) and on April 12, 2011 (https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2011/04/2011-04-11-03-mozilla-central/) it reported an incorrect value.

So, this is happening due to something on this list: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=09b605eb3e0d&tochange=a174b86200d6
Comment 3 Chris DeCairos (:cade) 2012-01-19 20:17:29 PST
this is a possible suspect: https://bugzilla.mozilla.org/show_bug.cgi?id=638807
Comment 4 Chris DeCairos (:cade) 2012-01-19 20:30:30 PST
yes I'm almost certain that bug 638807 is the culprit.
Comment 5 Chris DeCairos (:cade) 2012-01-23 15:43:39 PST
Created attachment 590908 [details] [diff] [review]
Added virtual keyword to RequesteFramBufferLength
Comment 6 Chris DeCairos (:cade) 2012-01-23 19:35:40 PST
Created attachment 590979 [details] [diff] [review]
Created a test for mozFrameBufferLength changes

This adds a test for the fix in attachment 590908 [details] [diff] [review]
Comment 7 Matthew Gregan [:kinetik] 2012-01-23 19:48:58 PST
Comment on attachment 590908 [details] [diff] [review]
Added virtual keyword to RequesteFramBufferLength

Great, thanks!

Tip for next time: it's best to request review from a specific person when setting the r? flag--that will cause a notification to be generated for that person, and it will add the request to their queue.
Comment 8 Matthew Gregan [:kinetik] 2012-01-23 19:51:10 PST
One other tip: please write the commit message in your patch in the active voice, and list the bug number at the beginning, e.g.:

"Bug 686137 - Make nsMediaDecoder::RequestFrameBufferLength virtual.  r=kinetik"
Comment 9 Chris DeCairos (:cade) 2012-01-23 19:54:53 PST
Ah, great I'll remember that! Thanks.
Comment 10 Matthew Gregan [:kinetik] 2012-01-23 20:02:48 PST
Comment on attachment 590979 [details] [diff] [review]
Created a test for mozFrameBufferLength changes

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

I'll r+ once the first item below is fixed--the others are not required, just nice to tidy up.

::: content/media/test/Makefile.in
@@ +154,5 @@
>  		test_volume.html \
>  		test_video_to_canvas.html \
>  		use_large_cache.js \
>  		test_audiowrite.html \
> +		test_bug686137.html \

This test includes an Ogg file, so it needs to be inside the set of _TEST_FILES protected by the MOZ_OGG conditional.

::: content/media/test/test_bug686137.html
@@ +12,5 @@
> +<body>
> +<a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=686137">Mozilla Bug 686137</a>
> +
> +<!-- mute audio, since there is no need to hear the sound for these tests -->
> +<audio id='a1' controls></audio>

Remove the comment about muting the audio and add "muted" to the audio declaration, then remove the |a1.muted = true| in metaDataLoaded.  I know this bit probably came from a similar test, but we recently gained support for the "muted" attribute, so it's clearer use that than have a comment and mute the element elsewhere in the code.

@@ +21,5 @@
> +var a1 = document.getElementById('a1');
> +
> +function audioAvailable(event) {
> +  a1.removeEventListener("MozAudioAvailable", audioAvailable);
> +  is( event.frameBuffer.length, a1.mozFrameBufferLength, "event.frameBuffer.length should be " + a1.mozFrameBufferLength + ".");

Maybe also test that event.frameBuffer.length is equal to the value you set earlier, to catch the case where setting mozFrameBufferLength could be ignored/lost.

@@ +38,5 @@
> +  a1.src = testFile;
> +  a1.load();
> +}
> +
> +window.addEventListener("load", function(e) {

It's not necessary to wait for the load event and run initTest() off of it, you can just execute the body of initTest() immediately here--the test only depends on the "a1" audio element existing in the DOM before the script runs, which is guaranteed by the order the audio element and script appear in the document.
Comment 11 Chris DeCairos (:cade) 2012-01-23 21:13:23 PST
Created attachment 590991 [details] [diff] [review]
Fixes for tests as per review

I think I've address all the points in your review.
Comment 12 Matthew Gregan [:kinetik] 2012-01-23 21:30:21 PST
Comment on attachment 590991 [details] [diff] [review]
Fixes for tests as per review

Thanks!
Comment 13 Chris DeCairos (:cade) 2012-01-24 08:27:58 PST
Created attachment 591110 [details] [diff] [review]
Make nsMediaDecoder::RequestFrameBufferLength virtual

Cleaned up the commit message
Comment 14 Chris DeCairos (:cade) 2012-01-24 08:30:19 PST
Comment on attachment 591110 [details] [diff] [review]
Make nsMediaDecoder::RequestFrameBufferLength virtual

shouldn't have put that r+ in there

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