Setting audio.mozFrameBufferLength has no effect on MozAudioAvailable.frameBuffer.length

RESOLVED FIXED in mozilla12

Status

()

defect
RESOLVED FIXED
8 years ago
7 years ago

People

(Reporter: humph, Assigned: cade)

Tracking

({regression})

Trunk
mozilla12
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

Reporter

Description

8 years ago
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"
Reporter

Updated

8 years ago
OS: Mac OS X → All
Hardware: x86 → All
Assignee

Comment 1

8 years ago
I can look into fixing this :D
Reporter

Updated

8 years ago
Assignee: nobody → chris
Status: NEW → ASSIGNED
Assignee

Comment 2

7 years ago
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
Assignee

Comment 4

7 years ago
yes I'm almost certain that bug 638807 is the culprit.
Reporter

Updated

7 years ago
Depends on: 638807
Assignee

Updated

7 years ago
Attachment #590908 - Flags: feedback?
Assignee

Comment 6

7 years ago
This adds a test for the fix in attachment 590908 [details] [diff] [review]
Attachment #590979 - Flags: review?
Assignee

Updated

7 years ago
Attachment #590908 - Flags: feedback? → review?
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.
Attachment #590908 - Flags: review? → review+
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"
Assignee

Comment 9

7 years ago
Ah, great I'll remember that! Thanks.
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.
Attachment #590979 - Flags: review?
Assignee

Comment 11

7 years ago
I think I've address all the points in your review.
Attachment #590979 - Attachment is obsolete: true
Attachment #590991 - Flags: review?(kinetik)
Comment on attachment 590991 [details] [diff] [review]
Fixes for tests as per review

Thanks!
Attachment #590991 - Flags: review?(kinetik) → review+
Assignee

Comment 13

7 years ago
Cleaned up the commit message
Attachment #590908 - Attachment is obsolete: true
Attachment #591110 - Flags: review+
Assignee

Comment 14

7 years ago
Comment on attachment 591110 [details] [diff] [review]
Make nsMediaDecoder::RequestFrameBufferLength virtual

shouldn't have put that r+ in there
Attachment #591110 - Flags: review+
Assignee

Updated

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