Closed Bug 686137 Opened 13 years ago Closed 13 years ago

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

Categories

(Core :: Audio/Video, defect)

Product:
Core
Component:
Audio/Video
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla12

People

(Reporter: humph, Assigned: cade)

References

Details

(Keywords: regression)

Attachments

(2 files, 2 obsolete files)

Fixes for tests as per review
2.11 KB, patch
kinetik
: review+
Details | Diff | Splinter Review
Make nsMediaDecoder::RequestFrameBufferLength virtual
1.12 KB, patch
Details | Diff | Splinter Review
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"
OS: Mac OS X → All
Hardware: x86 → All
I can look into fixing this :D
Assignee: nobody → chris
Status: NEW → ASSIGNED
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
this is a possible suspect: https://bugzilla.mozilla.org/show_bug.cgi?id=638807
yes I'm almost certain that bug 638807 is the culprit.
Depends on: 638807
Attachment #590908 - Flags: feedback?
This adds a test for the fix in attachment 590908 [details] [diff] [review]
Attachment #590979 - Flags: review?
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"
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?
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+
Cleaned up the commit message
Attachment #590908 - Attachment is obsolete: true
Attachment #591110 - Flags: review+
Comment on attachment 591110 [details] [diff] [review] Make nsMediaDecoder::RequestFrameBufferLength virtual shouldn't have put that r+ in there
Attachment #591110 - Flags: review+
Keywords: regression → checkin-needed
Keywords: regression
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: