Closed Bug 635649 Opened 9 years ago Closed 9 years ago

Refactor Wave backend to use content/media nsBuiltinDecoder framework


(Core :: Audio/Video, defect)

Not set





(Reporter: cajbir, Assigned: cajbir)




(2 files, 6 obsolete files)

The wave backend uses an older method whereby almost everything to do with the audio element spec is handled by the backend itself. Since it was written we've refactored our common media code so it is easier to write decoders that pass the various tests for spec compliance. We should redo the wave backend to use this common code.
Assignee: nobody → chris.double
Attached patch Fix (obsolete) — Splinter Review
Re-implements the wave backend on top of nsBuiltinDecoder. Patch based on Matthew's original work on doing this.
Attachment #513919 - Flags: review?(kinetik)
Comment on attachment 513919 [details] [diff] [review]

There was a build error on maemo. I'm fixing and will attach a new patch.
Attachment #513919 - Flags: review?(kinetik)
Attached patch Fix (obsolete) — Splinter Review
Adds fix for maemo builds
Attachment #513919 - Attachment is obsolete: true
Attachment #514399 - Flags: review?(kinetik)
Blocks: 580461
+  // If the Theora granulepos has not been captured, it may read several packets
+  // until one with a granulepos has been captured, to ensure that all packets
+  // read have valid time info.
+  virtual PRBool DecodeVideoFrame(PRBool &aKeyframeSkip,
+                                  PRInt64 aTimeThreshold);

This needs a more appropriate comment.

+namespace {
+  static PRUint32
+  ReadUint32BE(const char** aBuffer)

Static is redundant with an anonymous namespace.

+  mInfo.mHasAudio = PR_TRUE;
+  mInfo.mHasVideo = PR_FALSE;
+  mInfo.mAudioRate = mSampleRate;
+  mInfo.mAudioChannels = mChannels;

Set mDataOffset to -1 here like we do for WebM.

+  static const PRInt64 BLOCK_SIZE = 512;

This seems very small (~2.9ms of audio for CD quality).  Is it safe to make it something larger, say 4096?

+  PRInt64 samples = readSize / mSampleSize;

Remove extra newline.

+        *s++ = v * (1.F/PR_INT8_MAX) * MOZ_MAX_SAMPLE_VALUE;

This isn't the correct conversion for UINT8 to SINT16/FLOAT32.  It only produces positive values.  See nsAudioStream::Write and MOZ_CONVERT_SOUND_SAMPLE.

+      else if (mSampleFormat == nsAudioStream::FORMAT_S16_LE) {  

Remove trailing spaces.

The comments for BytesToTime, TimeToBytes, and RoundDownToSample should be moved to the header file.
Attached patch Fix (obsolete) — Splinter Review
Address review comments.
Attachment #514399 - Attachment is obsolete: true
Attachment #514399 - Flags: review?(kinetik)
Attachment #514972 - Flags: review?(kinetik)
+#elif defined(MOZ_SAMPLE_TYPE_FLOAT32)
+        *s++ = (PRInt32(v) - PR_INT16_MIN) / float(PR_UINT16_MAX);

Float32 PCM samples should have a range of [-1.0, 1.0], but this conversion seems to result in [0.0, 1.0].

Since this patch now gives us a working Audio Data API for Wave files, we can test these conversions by adding the appropriate files and checking the data received from MozAudioAvailable.  test_a4_tone.html is an existing example, but that's far more complex than we'd need to cover these cases.
Attached patch Fix (obsolete) — Splinter Review
Addresses float conversion review comment and adds test. The test check for three values from a u8 and s16 wav file. These three values represent max, min and middle sound values.
Attachment #514972 - Attachment is obsolete: true
Attachment #514972 - Flags: review?(kinetik)
Attachment #517497 - Flags: review?(kinetik)
Attachment #517497 - Flags: review?(kinetik) → review+
Whiteboard: [needs landing post ff4]
(In reply to comment #8)
> Created attachment 517497 [details] [diff] [review]
> Fix

I get mochitest timeouts in toolkit/content/tests/widgets/test_videocontrols_video_direction.html and toolkit/content/tests/widgets/test_videocontrols_audio_direction.html with this patch applied.

I tracked down the problem to ensure it wasn't caused by the other patches I had applied...

The problem is that when nsWaveReader::GetBuffered() is calculating its buffered ranges, it calculates the end time of a range as a float in seconds as:
BytesToTime(endOffset - mWavePCMOffset) (nsWaveReader.cpp:269)

But when we set the duration of the media in nsWaveReader::ReadMetadata(), we call
mDecoder->GetStateMachine()->SetDuration(BytesToTime(GetDataLength()) * 1000);
SetDuration takes a PRInt64 argument, so the implicit type conversion rounds down.

So duration is rounded down to the nearest ms, whereas the buffered range times are floats in seconds. So a WAV's buffered range can end a fraction of a ms later than the wav's reported duration.

This causes test_videocontrols_*_direction.html to time out. Those tests terminate when a media's last progress event fires, if (buffered.length == 1 && buffered.end(0) == duration).

But for toolkit/content/tests/widgets/audio.wav, with the current patch applied, buffered.end(0) == 0.03124716505408287 and duration == 0.031, so since buffered.end(0) is never exactly duration, the test times out.

The old nsWaveStateMachine::GetDuration() [] didn't round off the duration to the nearest ms, so this problem didn't exist in the old Wave backend.

Two possible ways to fix this:
1. Round the wave buffered range times to the nearest ms, similar how to the nsOggReader does this []. This is easy. Or,
2. Change nsBuiltin*Decoder to not round times/durations reported to JS to the nearest ms.

Firefox is the only browser that rounds the media element's duration to the nearest ms. Chrome, Safari, IE9b and Opera are returning un-rounded floating point numbers for duration.

Maybe we should do 1 now, until such a time as we can do 2?
Interesting. I wonder why I didn't get test failures - is it Windows only or intermittent?

Can we change the nsBuiltinDecoder code to detect this failure in backends so we don't have to rely on test failures?
There's nothing wrong with relying on test failures :-).

Why can't we do #2 right now?
I only noticed this when it failed for me on try.

We can do #2 now, it's a bit more work. I'll put a patch together, since it blocks my other work.
Hmm, we store timestamps internally as PRInt64 in ms, so if we did #2 we'd want to change all our timestamping code to use doubles, else we'd not get better than ms precision anyway.

We used to use doubles to store timestamps, but moved away when we dropped liboggplay, to prevent floating point rounding errors causing seeks to miss keyframes (see bug 531340 comment 5). I think if we're more careful to not allow errors to compound, we probably won't hit that problem again, but I'd need to test to be sure.
We could use store usec timestamps in PRInt64.  I'm strongly against using floating point for this.
I agree, this gets pretty tricky: on x86-32, for example, gcc will sometimes compare against 80-bit floating point numbers it happens to have in a register, and sometimes use 64-bit numbers it's stored back to RAM, and you really have no way to control when that happens (other than forcing it to always store everything back for every operation, which is a cure worse than the disease). So it is very difficult to write code that depends on exact, or even consistent comparison results.

I believe gstreamer, for example, takes the approach of storing timestamps in the timebase associated with the stream (e.g., frames for video, samples for audio, etc.), and uses some extended-precision arithmetic for comparisons of timestamps in different timebases. This is more work, but ensures there are never any rounding errors.
Microseconds it is. I'm keen to store audio times in samples rather than time, since everywhere we need fine control we're counting in samples anyway, and that would fix bug 634747 too.
(In reply to comment #8)
> Created attachment 517497 [details] [diff] [review]
> Fix

I see another test failure with this patch, but not on all platforms:

59302 ERROR TEST-UNEXPECTED-FAIL | /tests/content/media/test/test_wave_data_u8.html | First sound sample should be 1.0. It was 1.0000001192092896
Took off landing flag until I rebase and investigate comment 17.
Whiteboard: [needs landing post ff4] → ]
Attached patch Fix (obsolete) — Splinter Review
Rebased to trunk. Carry forward review.
Attachment #517497 - Attachment is obsolete: true
Attachment #521726 - Flags: review+
Whiteboard: ]
Attached patch Fix (obsolete) — Splinter Review
Minor changes:

- Addresses comment 9 by taking the first option (round buffered ranges to millisecond like the ogg backend). This should suffice until bug 641718 is landed.
- Addresses comment 17. Try server is green.
- Minor changes to remove Windows build warnings. Waiting for try server results on this to make sure it's still green.
Attachment #521726 - Attachment is obsolete: true
Attachment #522579 - Flags: review?(kinetik)
Comment on attachment 522579 [details] [diff] [review]

And tryserver shows orange, sorry kinetik.
Attachment #522579 - Flags: review?(kinetik)
Attached patch FixSplinter Review
Green on try, finally.
Attachment #522579 - Attachment is obsolete: true
Attachment #522888 - Flags: review?(kinetik)
Attachment #522888 - Flags: review?(kinetik) → review+
This fixes test_autoplay_contentEditable test so that canplay can fire multiple times in a row. When the ready state changes from HAVE_FUTURE_DATA to HAVE_CURRENT_DATA and back to HAVE_FUTURE_DATA and we are not playing then it can fire twice.
Attachment #522929 - Flags: review?(kinetik)
Attachment #522929 - Flags: review?(kinetik) → review+
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.2
(In reply to comment #22)
> Created attachment 522888 [details] [diff] [review]

>diff --git a/content/media/test/test_wave_data_s16.html b/content/media/test/test_wave_data_s16.html
>+    ok(samples[1] > -1.01 && samples [1] < 0.01, "Second sound sample should be close to -1.0. It was " + samples[1]);

Was this meant to be:
ok(samples[1] > -1.01 && samples [1] < -0.99, "...")

>diff --git a/content/media/test/test_wave_data_u8.html b/content/media/test/test_wave_data_u8.html
>+    ok(samples[1] > -1.01 && samples [1] < 0.01, "Second sound sample should be close to -1.0. It was " + samples[1]);

And here too?
Yes, good catch.
Raised bug 646682 for the issue in comment 25. Thanks Chris!
Can someone confirm if this is fixed?
You need to log in before you can comment on or make changes to this bug.