The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in mozilla12

Status

()

Core
Audio/Video
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: humph, Assigned: cade)

Tracking

({regression})

Trunk
mozilla12
regression
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

6 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

6 years ago
OS: Mac OS X → All
Hardware: x86 → All
(Assignee)

Comment 1

5 years ago
I can look into fixing this :D
(Reporter)

Updated

5 years ago
Assignee: nobody → chris
Status: NEW → ASSIGNED
(Assignee)

Comment 2

5 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 3

5 years ago
this is a possible suspect: https://bugzilla.mozilla.org/show_bug.cgi?id=638807
(Assignee)

Comment 4

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

Updated

5 years ago
Depends on: 638807
(Assignee)

Comment 5

5 years ago
Created attachment 590908 [details] [diff] [review]
Added virtual keyword to RequesteFramBufferLength
(Assignee)

Updated

5 years ago
Attachment #590908 - Flags: feedback?
(Assignee)

Comment 6

5 years ago
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]
Attachment #590979 - Flags: review?
(Assignee)

Updated

5 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

5 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

5 years ago
Created attachment 590991 [details] [diff] [review]
Fixes for tests as per review

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

5 years ago
Created attachment 591110 [details] [diff] [review]
Make nsMediaDecoder::RequestFrameBufferLength virtual

Cleaned up the commit message
Attachment #590908 - Attachment is obsolete: true
Attachment #591110 - Flags: review+
(Assignee)

Comment 14

5 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

5 years ago
Keywords: regression → checkin-needed
Keywords: regression
http://hg.mozilla.org/integration/mozilla-inbound/rev/f684bc455353
http://hg.mozilla.org/integration/mozilla-inbound/rev/ab66c3d65c04
Keywords: checkin-needed
Target Milestone: --- → mozilla12
https://hg.mozilla.org/mozilla-central/rev/ab66c3d65c04
https://hg.mozilla.org/mozilla-central/rev/f684bc455353
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.