Refactor Wave backend to use content/media nsBuiltinDecoder framework

VERIFIED FIXED in mozilla5

Status

()

Core
Audio/Video
VERIFIED FIXED
7 years ago
7 years ago

People

(Reporter: cajbir, Assigned: cajbir)

Tracking

Trunk
mozilla5
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 6 obsolete attachments)

(Assignee)

Description

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

Updated

7 years ago
Assignee: nobody → chris.double
Status: NEW → ASSIGNED
(Assignee)

Comment 1

7 years ago
Created attachment 513919 [details] [diff] [review]
Fix

Re-implements the wave backend on top of nsBuiltinDecoder. Patch based on Matthew's original work on doing this.
Attachment #513919 - Flags: review?(kinetik)
(Assignee)

Comment 2

7 years ago
Comment on attachment 513919 [details] [diff] [review]
Fix

There was a build error on maemo. I'm fixing and will attach a new patch.
Attachment #513919 - Flags: review?(kinetik)
(Assignee)

Comment 3

7 years ago
Created attachment 514399 [details] [diff] [review]
Fix

Adds fix for maemo builds
Attachment #513919 - Attachment is obsolete: true
Attachment #514399 - Flags: review?(kinetik)
Mmmm, massive code reduction!
(Assignee)

Updated

7 years ago
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;
+
+
+  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.
(Assignee)

Comment 6

7 years ago
Created attachment 514972 [details] [diff] [review]
Fix

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.
(Assignee)

Comment 8

7 years ago
Created attachment 517497 [details] [diff] [review]
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.
Attachment #514972 - Attachment is obsolete: true
Attachment #514972 - Flags: review?(kinetik)
Attachment #517497 - Flags: review?(kinetik)
Attachment #517497 - Flags: review?(kinetik) → review+
(Assignee)

Updated

7 years ago
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() [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?
(Assignee)

Comment 10

7 years ago
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.
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.
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.
Depends on: 641718
(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
(Assignee)

Comment 18

7 years ago
Took off landing flag until I rebase and investigate comment 17.
Whiteboard: [needs landing post ff4] → ]
(Assignee)

Comment 19

7 years ago
Created attachment 521726 [details] [diff] [review]
Fix

Rebased to trunk. Carry forward review.
Attachment #517497 - Attachment is obsolete: true
Attachment #521726 - Flags: review+
(Assignee)

Updated

7 years ago
Whiteboard: ]
(Assignee)

Comment 20

7 years ago
Created attachment 522579 [details] [diff] [review]
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.
Attachment #521726 - Attachment is obsolete: true
Attachment #522579 - Flags: review?(kinetik)
(Assignee)

Comment 21

7 years ago
Comment on attachment 522579 [details] [diff] [review]
Fix

And tryserver shows orange, sorry kinetik.
Attachment #522579 - Flags: review?(kinetik)
(Assignee)

Comment 22

7 years ago
Created attachment 522888 [details] [diff] [review]
Fix

Green on try, finally.
Attachment #522579 - Attachment is obsolete: true
Attachment #522888 - Flags: review?(kinetik)
Attachment #522888 - Flags: review?(kinetik) → review+
(Assignee)

Comment 23

7 years ago
Created attachment 522929 [details] [diff] [review]
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.
Attachment #522929 - Flags: review?(kinetik)
Attachment #522929 - Flags: review?(kinetik) → review+
(Assignee)

Comment 24

7 years ago
http://hg.mozilla.org/mozilla-central/rev/c158f8b0a921
Status: ASSIGNED → RESOLVED
Last Resolved: 7 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.
(Assignee)

Comment 27

7 years ago
Raised bug 646682 for the issue in comment 25. Thanks Chris!
Can someone confirm if this is fixed?
Yep.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.