635649 - Refactor Wave backend to use content/media nsBuiltinDecoder framework

Status: VERIFIED FIXED

(Core :: Audio/Video, defect)

Description

[cajbir (:cajbir)]

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.

Comment 1

[cajbir (:cajbir)]

Attachment: Fix

Re-implements the wave backend on top of nsBuiltinDecoder. Patch based on Matthew's original work on doing this.

Comment 2

[cajbir (:cajbir)]

Comment on attachment 513919 [details] [diff] [review]
Fix

There was a build error on maemo. I'm fixing and will attach a new patch.

Comment 3

[cajbir (:cajbir)]

Attachment: Fix

Adds fix for maemo builds

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Mmmm, massive code reduction!

Comment 5

[Matthew Gregan [:kinetik]]

+  // 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;
+
+
+  PR_STATIC_ASSERT(PRUint64(BLOCK_SIZE) < UINT_MAX / sizeof(SoundDataValue) / MAX_CHANNELS);

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.

Comment 6

[cajbir (:cajbir)]

Attachment: Fix

Address review comments.

Comment 7

[Matthew Gregan [: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.

Comment 8

[cajbir (:cajbir)]

Attachment: Fix

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.

Comment 9

[Chris Pearce [:cpearce (Not reading bugmail)]]

(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() [http://mxr.mozilla.org/mozilla-central/source/content/media/wave/nsWaveDecoder.cpp#482] 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 [http://mxr.mozilla.org/mozilla-central/source/content/media/ogg/nsOggReader.cpp#1683]. 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?

Comment 10

[cajbir (:cajbir)]

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?

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

There's nothing wrong with relying on test failures :-).

Why can't we do #2 right now?

Comment 12

[Chris Pearce [:cpearce (Not reading bugmail)]]

I only noticed this when it failed for me on try.
http://tbpl.mozilla.org/?tree=MozillaTry&rev=927b5d80c8d0

We can do #2 now, it's a bit more work. I'll put a patch together, since it blocks my other work.

Comment 13

[Chris Pearce [:cpearce (Not reading bugmail)]]

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.

Comment 14

[Matthew Gregan [:kinetik]]

We could use store usec timestamps in PRInt64.  I'm strongly against using floating point for this.

Comment 15

[Timothy B. Terriberry (:derf)]

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.

Comment 16

[Chris Pearce [:cpearce (Not reading bugmail)]]

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.

Comment 17

[Chris Pearce [:cpearce (Not reading bugmail)]]

(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

http://tbpl.mozilla.org/?tree=MozillaTry&rev=66b6b3800ba9

Comment 18

[cajbir (:cajbir)]

Took off landing flag until I rebase and investigate comment 17.

Comment 19

[cajbir (:cajbir)]

Attachment: Fix

Rebased to trunk. Carry forward review.

Comment 20

[cajbir (:cajbir)]

Attachment: Fix

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.

Comment 21

[cajbir (:cajbir)]

Comment on attachment 522579 [details] [diff] [review]
Fix

And tryserver shows orange, sorry kinetik.

Comment 22

[cajbir (:cajbir)]

Attachment: Fix

Green on try, finally.

Comment 23

[cajbir (:cajbir)]

Attachment: Fix for intermittent orange that the wave backend introduces

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.

Comment 24

[cajbir (:cajbir)]

http://hg.mozilla.org/mozilla-central/rev/c158f8b0a921

Comment 25

[Chris Pearce [:cpearce (Not reading bugmail)]]

(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?

Comment 26

[Matthew Gregan [:kinetik]]

Yes, good catch.

Comment 27

[cajbir (:cajbir)]

Raised bug 646682 for the issue in comment 25. Thanks Chris!

Comment 28

[u279076]

Can someone confirm if this is fixed?

Comment 29

[Matthew Gregan [:kinetik]]

Yep.