Looping <audio> files should be seamless

RESOLVED FIXED in Firefox 59

Status

()

P3
normal
RESOLVED FIXED
8 years ago
a month ago

People

(Reporter: huskyr, Assigned: chunmin, Mentored)

Tracking

(Depends on: 2 bugs, Blocks: 2 bugs)

unspecified
mozilla59
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(tracking-b2g:backlog, firefox59 fixed)

Details

(Whiteboard: [games:p3], URL)

Attachments

(10 attachments, 25 obsolete attachments)

1.59 MB, application/zip
Details
18.42 KB, patch
Details | Diff | Splinter Review
59 bytes, text/x-review-board-request
jwwang
: review+
Details
59 bytes, text/x-review-board-request
jwwang
: review+
Details
59 bytes, text/x-review-board-request
jwwang
: review+
Details
59 bytes, text/x-review-board-request
jwwang
: review+
Details
59 bytes, text/x-review-board-request
jwwang
: review+
Details
59 bytes, text/x-review-board-request
jwwang
: review+
Details
59 bytes, text/x-review-board-request
jwwang
: review+
Details
59 bytes, text/x-review-board-request
jwwang
: review+
Details
(Reporter)

Description

8 years ago
User-Agent:       Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0.1) Gecko/20100101 Firefox/4.0.1
Build Identifier: 4.0.1

When looping an <audio> file there's a small, audible, gap when re-starting the audio file from the beginning. 

This makes it impossible to make something like an audio sequencer or drum loops.

Currently the 'loop' HTML5 property is not implemented (see #449157), but Javascript like this should have the same effect:

<audio id="audio" src="loop.wav" loop autoplay controls />

<script>
document.getElementById('audio').addEventListener('ended', function() {
    this.currentTime = 0;
}, false);
</script>

Reproducible: Always
The current plan is to fix bug 449157 in such a way that the looping is seamless.  I'm not sure it's feasible to provide (or, at least, guarantee) seamless looping when script sets currentTime to the start of the media.
(Reporter)

Comment 2

8 years ago
Great, thanks for taking that into account. I'm just wondering: is there any technical difference in setting 'loop' to  true or giving a 'currentTime = 0' on an ended event? Seems the same to me...
There are, I forgot to mention them earlier.  One is that the loop attribute allows the decoder to know in advance that the media is looping, so it can prepare to handle that by not tearing down resources at the end of playback (such as the decode threads, audio device, etc.)

The second is that your example sets currentTime in an ended event handler.  The ended event is dispatched after playback ends, which is dispatched after we have drained and closed the audio device.  The event handler runs asynchronously on the main thread, so (in addition to the tearing down resources issue mentioned above) there is some delay between playback ending where the event is dispatched and the time the event handler runs.

To allow seamless looping, the ended event would need be to be dispatched early enough that the event handler would set currentTime at the correct moment, which given the indeterminate delays between the event dispatch and event handler running, isn't really possible.

Having said that, there are probably improvements that can be made to reduce the delay between dispatching the ended event and starting playback again.  It's just that it's not possible to guarantee anything close to seamless looping without the loop attribute.
(In reply to comment #3)
> It's just that it's not possible to guarantee anything close to seamless
> looping without the loop attribute.

Note also that when using lossy compression (such as Vorbis), even if the first sample of the file immediately follows the last, there may be glitches caused by differing quantization used on the two blocks. For truly seamless looping, you want to crosslap the end of the file with the beginning using the MDCT windows, which may require specially prepared files for best results (see http://xiph.org/vorbis/doc/vorbisfile/crosslap.html for details). This is something I expect would be done if the loop attribute is present, but may not be the behavior you would want on a normal seek.
Based on comment 1 I think it would be safe to add a dependency to bug 449157 or dupe it.
Whiteboard: DUPEME?
Status: UNCONFIRMED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 449157
Reopening this to track seamless looping.  Bug 449157 now tracks basic (non-seamless) looping support.
Status: RESOLVED → REOPENED
Depends on: 664918
Ever confirmed: true
Resolution: DUPLICATE → ---
Whiteboard: DUPEME?
Depends on: 449157
Created attachment 575792 [details]
testcase - 440Hz sine

Updated

7 years ago
Whiteboard: [games:p2]
Depends on: 782507

Comment 9

5 years ago
This still seems to be an issue on Linux at least.
(In reply to Alexander Brüning from comment #9)
> This still seems to be an issue on Linux at least.

No patches have landed to resolve this issue yet.

Comment 11

5 years ago
Hey, any progress here? We are getting bit by this issue as well.

It sure looks like audio loop scheduling implementation in audio.loop attribute does not even attempt to perform sample-precise joining, but plays the next instance of the audio loop to play back way too late after the first.

When I have an audio clip that looks like this: https://dl.dropboxusercontent.com/u/40949268/Bugs/firefox_audio_loop/Screen%20Shot%202014-05-16%20at%204.45.56%20PM.png

we are getting audio playback which when recorded back looks like this:

https://dl.dropboxusercontent.com/u/40949268/Bugs/firefox_audio_loop/Screen%20Shot%202014-05-16%20at%204.44.00%20PM.png

There is about 80msec gap in the scheduled playback between loops. I don't think this is a question about lossy compression - definitely the loop="true" attribute should in itself be able to schedule sample-precise joins of the audio data, and then it's the question of the audio data source on how well it sounds like. In that image, it looks like there's about 80 msecs of delay, which at a source audio data rate of 48kHz means that the audio buffer scheduling was off by some 3840 samples.

Here is a minimal STR with the loop="true" attribute: https://dl.dropboxusercontent.com/u/40949268/Bugs/firefox_audio_loop/audio_loop.html

Some suggested using the audio 'onended' event to fire the next loop. Here is a handwritten STR #2 for that workaround: https://dl.dropboxusercontent.com/u/40949268/Bugs/firefox_audio_loop/audio_loop_onended.html
which produces the same results. I agree with the earlier comments that this is not a good way to code, since the browser cannot prepare for this in advance.

I know that Web Audio API implements this properly, so at first I thought to use that, but I hit two obstacles:

  - currently we use HTMLAudioElements to load up audio files, so that they're loaded via browser WAV and OGG codecs. Looking here, http://updates.html5rocks.com/2012/02/HTML5-audio-and-the-Web-Audio-API-are-BFFs , it seems that <audio> elements integrate with Web Audio, but still, the playback api is not as flexible as when playing back AudioBufferSourceNodes? I was not able to start the playback in the Web Audio API graph and control looping nor manually schedule there. Is it possible to work around the looping some way with Web Audio API?

  - my other thought was to use Web Audio API with AudioBufferSourceNodes, but because of the above limitation, that means I would have to implement WAV and OGG codecs in JavaScript, just to be able to drive the looping from the Web Audio API graph. Or is there some other way to get the raw audio buffer data from <audio> so that one could use the same API as AudioBufferSourceNodes to play back?

Are there known workarounds for this issue?
Created attachment 8423887 [details]
loop.tar.gz

Web Audio API is perfectly suitable for this, see the attached demo (unzip, untar and run loop.html, the audio sample is included). I threw in a play/pause button, as it seemed to be a needed feature.

Note that you can put any codec that works in <audio> in the decodeAudioData call, it'll get decoded for you by the Web Audio API.

Jukka, does that work for you?
Flags: needinfo?(jujjyl)

Comment 13

5 years ago
Thanks, I downloaded the file, but I wonder if it has the wrong file? It did not contain a file loop.html.
Flags: needinfo?(jujjyl)
Created attachment 8423890 [details]
loop.tar.gz

Oops, went too fast, this has the right files.
Attachment #8423887 - Attachment is obsolete: true

Comment 15

5 years ago
Perfect, that workaround is working great! Thanks Paul!

Comment 16

5 years ago
Testing more, I am hitting this bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1012801

How difficult would this bug be to fix? (I expect between this and 1012801, this bug would be the easier one to tackle?) I seem to be trading bugs now when trying to find a workaround.

Updated

5 years ago
Blocks: 1014243

Updated

5 years ago
blocking-b2g: --- → 1.4?
Moving this to backlog.

This is too late to take in 1.4
blocking-b2g: 1.4? → backlog
Note: For games we need either this bug fixed or Bug 1012801 to be able to do seamlessly looping audio. Likely target is 2.0.
blocking-b2g: backlog → ---
tracking-b2g: --- → backlog
Component: Audio/Video → Audio/Video: Playback
Whiteboard: [games:p2] → [games:p?]

Comment 19

2 years ago
Looking back at this, currently in order to play back looping audio clips, one can
   1. decodeAudioData() the whole clip up front, and play it as a looping audio buffer with WebAudio (AudioBufferSourceNode.loop=true), which will be seamless
   2. Emscripten compile Ogg Vorbis and implement the asset decoding and streaming manually in a looping manner

The first one is good solution if the sound clip is really short so that decodeAudioData() will take very little time and very little memory. The second solution is often used in asm.js compiled games to keep the performance good and memory footprint low.

Can Web Audio API produce seamlessly looping audio while decoding on demand? I.e. not having to do decodeAudioData() of the whole clip? When Paul posted comment 14, I believe it was not possible then, but that was some time ago - what is the situation today?

In any case, marking as games:p3 for now, given that we do have options 1. and 2. that solve the most common cases.
Flags: needinfo?(padenot)

Updated

2 years ago
Whiteboard: [games:p?] → [games:p3]
(In reply to Jukka Jylänki from comment #19)
>    2. Emscripten compile Ogg Vorbis and implement the asset decoding and
> streaming manually in a looping manner

Not sure exactly how this handles seemless looping, but perhaps MSE could be used to manually streaming the encoded data, if that can be looped.

> Can Web Audio API produce seamlessly looping audio while decoding on demand?

Only through MediaElementAudioSourceNode, if a media element can play the looping audio.

Or maybe the assets can be broken in parts for the client to pass to decodeAudioData() shortly before required.
Yes, what karlt said, the situation has not changed much on the Web Audio API front.
Flags: needinfo?(padenot)

Comment 22

2 years ago
(In reply to Karl Tomlinson (ni?:karlt) from comment #20)
> (In reply to Jukka Jylänki from comment #19)
> > Can Web Audio API produce seamlessly looping audio while decoding on demand?
> 
> Only through MediaElementAudioSourceNode, if a media element can play the
> looping audio.

Can you elaborate on this a bit? See the testcase audio_loop.html in comment 11:

> <html><head></head>
> <body>
> <audio id='a' src="noise.ogg" loop="true" /> </audio>
> <p>Pressing play should generate seamlessly joined audio playback (modulo any potential snapping occurring from issues with the authored audio):
> <p><b>TECHNIQUE 1: The audio.loop="true" property.</b>
> <input type="button" onclick="document.getElementById('a').play();" value="PLAY">
> </body>
> </html>

That does not currently produce seamlessly looping audio. Is the playback technically any different if this <audio> element was wrapped to a MediaElementAudioSourceNode in WebAudio graph, or will the audio output result (codepath) be the same?
(In reply to Jukka Jylänki from comment #22)
> Is the playback
> technically any different if this <audio> element was wrapped to a
> MediaElementAudioSourceNode in WebAudio graph, or will the audio output
> result (codepath) be the same?

The output is the same.  MediaElementAudioSourceNode is only useful for looping if a media element can do that.  The only way it currently might be able to do that would be if the encoded data could be continually streamed into a MediaSource buffer.
Flags: needinfo?(ajones)
See Also: → bug 1371202
ChunMin,
Per discussion, please put this on your plate.
Flags: needinfo?(ajones) → needinfo?(cchang)
Priority: -- → P3
(Assignee)

Comment 25

a year ago
Sure.
Assignee: nobody → cchang
Flags: needinfo?(cchang)
Mentor: jwwang
(Assignee)

Comment 26

a year ago
My original plan is to change to seek's behavior first. Suppose the playing time is P and the audio file is already demuxed/decoded to time Q, and P < Q. In current seek's mechanism, wherever playing time is seeked, the whole decodeer state will be reset and the prior demuxed/decoded data will be useless. On the other hand, if we don't reset the decoder when the playing time is seeked to R, where P <= R <= Q, then the silence-gap should be shorter slightly. 

This model can also be applied to this case if we think this bug is same as improving seeking's speed. Currently, the loop is implemented by seeking playing time to 0 when it finishs playing. If the demuxer/decoder can loop back to the beginning of the audio file to process when the file is demuxed/decoded to the end, then when the playing time is seeked to 0, the decoder state won't be reset because the demuxed/decoded time Q' is definitely greater than 0.

After brief discussion with JW, he offers a simpler solution for this bug. The playing is controlled by MediaDecoderStateMachine and the demuxer/decoder is controlled by MediaFormatReader. We can seek MediaFormatReader to the beginning in advance when the file is demuxed/decoded to the end, so when MediaDecoderStateMachine is seeking back to the beginning, the time for waiting decoded data should be saved.
(Assignee)

Comment 27

a year ago
Created attachment 8895727 [details]
seamless-loop-sample.wav

I extract one part from one of the samples on http://wavy.audio/
There is a fade-in and fade-out in the wave of the sample.

By my testing, Chrome cannot loop audio seamlessly, but Edge can.
(Assignee)

Comment 28

a year ago
Created attachment 8896860 [details]
loop-log.zip

Summary for loop.log
=================================
The normal time difference between data callback is 0.01 sec.

EOS		6.922
		  |  2.078
Drain		8.900
		  |  0.002
Seek		8.902
                  |
		  |  0.09
                  |
Audio start	8.993
		  |  0.002
First callback	9.000

- We may have 2 second to do things in advance(between EOS to Drain).
- The goal is to make the time difference between Seek and Audio-start closer to 0.01 sec.
(Assignee)

Comment 29

a year ago
(In reply to Chun-Min Chang[:chunmin] from comment #28)

> Summary for loop.log
> =================================
> The normal time difference between data callback is 0.01 sec.
> 
> EOS		6.922
> 		  |  2.078
> Drain		8.900
> 		  |  0.002
> Seek		8.902
>                 |
> 		  |  0.09
>                 |
> Audio start	8.993
> 		  |  0.002
> First callback	9.000
> 
> - We may have 2 second to do things in advance(between EOS to Drain).
> - The goal is to make the time difference between Seek and Audio-start
>   closer to 0.01 sec.
From the log below, we could estimate how much time we need from start requesting audio data to start playing it(prerolling). It needs 0.082 sec. and 0.076 sec. respectively, so we should have enough time(2.078 sec.) to request the audio data in advance.

1:
> 2017-08-14 06:32:06.592000 UTC - [MediaPlayback #2]: D/MediaDecoder Decoder=15532650 state=DECODING_METADATA change state to: DECODING_FIRSTFRAME
> ...
> 2017-08-14 06:32:06.592000 UTC - [MediaPlayback #1]: V/MediaFormatReader MediaFormatReader(0D7FD000)::RequestAudioData: 
> ...
> 2017-08-14 06:32:06.674000 UTC - [MediaPlayback #2]: D/MediaDecoder Decoder=15532650 MaybeStartPlayback() starting playback
> ...
> 2017-08-14 06:32:06.674000 UTC - [MediaPlayback #2]: D/AudioStream 1555C6A0 mozilla::AudioStream::Init channels: 2, rate: 44100
> ...
=> 6.674 - 6.592 = 0.082

2:
> 2017-08-14 06:32:08.903000 UTC - [MediaPlayback #2]: D/MediaDecoder Decoder=15532650 state=SEEKING change state to: DECODING
> ...
> 2017-08-14 06:32:08.903000 UTC - [MediaPlayback #2]: V/MediaFormatReader MediaFormatReader(0D7FD000)::RequestAudioData: 
> ...
> 2017-08-14 06:32:08.978000 UTC - [MediaPlayback #3]: D/MediaDecoder Decoder=15532650 MaybeStartPlayback() starting playback
> ...
> 2017-08-14 06:32:08.979000 UTC - [MediaPlayback #3]: D/AudioStream 1555C880 mozilla::AudioStream::Init channels: 2, rate: 44100
> ...
=> 8.979 - 8.903 = 0.076
(Assignee)

Comment 30

a year ago
Created attachment 8896903 [details]
test page and its logs

Keep the same logs but adding the test-page
Attachment #8896860 - Attachment is obsolete: true
You can't make assumption about the time it takes to open an audio stream. We have been measuring it in the field using telemetry probes, and it can take up to 8 seconds, with a very wide distribution.

It seems particularly wasteful to re-create an audio stream each time we loop here.
(Assignee)

Comment 32

a year ago
(In reply to Paul Adenot (:padenot) from comment #31)
Actually, I am interesting in how many time prerolling needs[0]. After it finishes, the playback start playing and initializing the audio stream. That's why I use it as checkpoint.

Yes, it would be better if we keep using the original audio stream while looping.

[0] http://searchfox.org/mozilla-central/rev/e5b13e6224dbe3182050cf442608c4cb6a8c5c55/dom/media/MediaDecoderStateMachine.cpp#833
[1] http://searchfox.org/mozilla-central/rev/e5b13e6224dbe3182050cf442608c4cb6a8c5c55/dom/media/MediaDecoderStateMachine.cpp#686
(Assignee)

Comment 33

a year ago
Created attachment 8897799 [details]
Dirty test

By comment 28, the bottleneck is demuxing/decoding audio data, so I try to preload the audio data to see how it could help.

For testing, I pre-request the audio data in completed-state and save them into another queue. After seeking to the beginning(looping), the pre-loaded audio data will be pushed into the original audio queue.

The time is shorten from 0.09 to 0.016 by this way. However, the delay is still detectable. The goal is to make it shorten to 0.01 at least.

Summary for loop.log
=================================
EOS		59.507
		  |  1.98
Drain		01.487
		  |  0.001
Seek		01.488
		  |  *0.016*
Audio start	01.504
		  |  0.002
First callback	01.506
(Assignee)

Comment 34

a year ago
Created attachment 8898665 [details]
Dirty test

Instead of pushing and popping the pre-loaded audio data (attachment 8897799 [details], comment 33), using pointers to the original audio queue and the pre-loaded audio queue then swaping them should be faster. In this test, it indeed shorten the time from seek to start the audio stream. However, it's weird that the time, from initializing audio stream to first callback, is increased. It doesn't happen in previous tests(comment 28 and comment 33). We should dig out what the cause is.

Maybe we could duplicate a MediaFormatReader to preload the audio and pre-initialize the audio stream stuff, then directly using the duplicated one for the next play.


Summary of test 1
----------------------------------
EOS		20.371
		  |  1.98
Drain		22.351
		  |  0
Seek		22.351
		  |  0.002
Audio start	22.353
		  |  *0.016*
First callback	22.369


Summary of test 2
----------------------------------
EOS		51.747
		  |  1.98
Drain		53.727
		  |  0.001
Seek		53.728
		  |  0.001
Audio start	53.729
		  |  *0.016*
First callback	53.745
Attachment #8897799 - Attachment is obsolete: true
(Assignee)

Comment 35

a year ago
Created attachment 8900158 [details]
dirty-test.zip

(In reply to Chun-Min Chang[:chunmin] from comment #34)
Correct the result. The right one should be:

Summary of test 1
----------------------------------
EOS		20.371
		  |  1.98
Drain		22.351
		  |  0
Seek		22.351
		  |  0.002
Audio Init	22.353
                  |  *0.014*
Audio start     22.367
		  |  0.002
First callback	22.369

and 

Summary of test 2
----------------------------------
EOS		51.747
		  |  1.98
Drain		53.727
		  |  0.001
Seek		53.728
		  |  0.001
Audio Init	53.729
                  |  *0.014*
Audio start	53.743
		  |  0.002
First callback	53.745

Thus, the bottleneck now is to open a cubeb stream. We should re-use it or pre-create it.
Attachment #8898665 - Attachment is obsolete: true
(Assignee)

Comment 36

a year ago
Created attachment 8900171 [details]
Pre-request audio data and pre-create audiostream in completed state

Summary for loop.log
=================================
The normal time difference between data callback is 0.01

EOS		11.468
		  |  1.98
Drain		13.448
		  |  0.001
Seek		13.449
		  |  0.002
Audio start     13.451
		  |  0.006
First callback	13.457

By pre-requesting audio data into a preloaded queue in completed state, and pre-create the audiostream if the preloaded queue is finished, we could shorten the delay to make seamless-looping work. On my PC, the seamless looping works well at first, but I can detect some delay after it plays for a while. Maybe it still has some bugs.

The patch is quite dirty but it indicates a direction we can keep working on:
1. Pre-requesting audio data after decoding state finishes
2. Pre-creating an audiostream if it's possible. The creation takes 12ms at least.
Attachment #8900158 - Attachment is obsolete: true
We really need to keep the same cubeb stream here, it will be more reliable. JW, how hard would this be ?
Flags: needinfo?(jwwang)
I think the major effort in in libcubeb which requires some API changes:
1. add cubeb_stream_pause/resume to replace the current use of cubeb_stream_start/stop
2. cubeb_stream_stop should reset playback position such that cubeb_stream_get_position calls after cubeb_stream_stop will get a position starting from 0.
Flags: needinfo?(jwwang)
(In reply to JW Wang [:jwwang] [:jw_wang] from comment #38)
> I think the major effort in in libcubeb which requires some API changes:
> 1. add cubeb_stream_pause/resume to replace the current use of
> cubeb_stream_start/stop
> 2. cubeb_stream_stop should reset playback position such that
> cubeb_stream_get_position calls after cubeb_stream_stop will get a position
> starting from 0.

I meant for looping: we really don't want to pause/play or reset the cubeb playback position when looping. We want to adjust the `currentTime` property, though, by doing some sort of modulo on the cubeb playback position with adjustments for the resampling latency and tail (if applicable).
The adjustment will introduce inaccuracy for it is hard to predict the final position of one loop when resampling or tailing is concerned. And the error will increase as we loop more times. I am not sure if this error will become significant/noticeable when we loop many times.
It's not too hard, we have the input and output latency of the resampler.

`pausing` and `resuming` at the audio stream level are expensive and sometimes blocking operations, I'd rather not go this route.
(In reply to Paul Adenot (:padenot) from comment #41)
> It's not too hard, we have the input and output latency of the resampler.
Since resampling is an internal detail to libcubeb and always introduces inaccuracy, I think AudioStream can adjust the position by calculating how many frames (before resampling) are sent to libcubeb callbacks if it knows the total number of frames of a loop.
(Assignee)

Comment 43

a year ago
Hi Paul,
Does WebAudio keep using same cubeb stream while looping? How to deal with the overflow of the variable tracking the total written frame?
Flags: needinfo?(padenot)
It is like MSG is playing an infinite stream which doesn't know about looping at all. Looping is handled by a WebAudio node here: http://searchfox.org/mozilla-central/rev/51b3d67a5ec1758bd2fe7d7b6e75ad6b6b5da223/dom/media/webaudio/AudioBufferSourceNode.cpp#508.
(Assignee)

Comment 45

a year ago
(In reply to JW Wang [:jwwang] [:jw_wang] from comment #44)
I mean, if we keep using the same stream to play infinite loop and don't reset the total_frames_written[0](for playback position), won't it overflow? We may calculate the position by some sort of modulo, but don't we need to reset it to prevent overflow?

[0] https://searchfox.org/mozilla-central/rev/51b3d67a5ec1758bd2fe7d7b6e75ad6b6b5da223/media/libcubeb/src/cubeb_wasapi.cpp#2052
It will overflow in the end... after playing more than 1000 years without stopping.
(Assignee)

Comment 47

a year ago
Good, then I don't need to care the bound of overflow if I want to keep using the same stream.
Flags: needinfo?(padenot)
(Assignee)

Comment 48

a year ago
Created attachment 8904178 [details]
[WIP] dirty implementation

The total delay is always 9ms like:

EOS
 | 1.98
Drain
 | 0
Seek
 | 0.003
Audio start
 | 0.006
First callback

Total delay = 0.009s = 9ms < 10ms

I'll try keeping using same audiostream next.
Attachment #8900171 - Attachment is obsolete: true
(Assignee)

Comment 49

a year ago
(In reply to Chun-Min Chang[:chunmin] from comment #48)
Under high-cpu-usage situation(80%-90%), the delay from first three rounds are 7ms, 8ms, 11ms. I don't collect the delay from other rounds since the results are all calculated manually. However, from my listening, I think most of delay are undetectable.
(Assignee)

Comment 50

a year ago
Created attachment 8904934 [details]
delay-checker.py

Parser for log file.

Usage example:
$ MOZ_LOG="MediaFormatReader:5, MediaDecoder:5, AudioStream:5, AudioSinkWrapper:5, timestamp" ./mach run > loop.log 2>&1
$ python delay-checker.py loop.log
(Assignee)

Comment 51

a year ago
The WIP patch works fine on Windows. It decodes data and creates the audio stream in advance in completedstate. 
However, it fails on OSX. The first audio callback needs more than 12ms from calling start, no matter the stream is created in advance or not. The creation of audio stream on Windows takes more than 10ms, while it only takes 2 ~ 4 ms, so the current patch doesn't work. I think I really need to try keeping the same stream here.

Summary of the testing log
-------------------------------------- 
EOS
  | 2.023518 = 2023.518 ms
Drain 
  | 0.000692 = 0.6 ms
Seek 
  | 0.001610 = 1.61 ms
Audio start
  | 0.012232 = 12.232 ms
First callback

Total delay: 0.014534 s = 14.534ms > 10ms
(Assignee)

Comment 52

a year ago
Created attachment 8904952 [details]
[WIP] dirty implementation: Pre-create stream
Attachment #8904178 - Attachment is obsolete: true
(Assignee)

Comment 53

a year ago
Created attachment 8905345 [details]
test page

Testing with homemade beats sample is not accurate enough, so I add sine wave audio in the tests. From testing with sine wave, I can hear a glitch that I cannot detect in the original beats sample.
Attachment #575792 - Attachment is obsolete: true
Attachment #8895727 - Attachment is obsolete: true
Attachment #8896903 - Attachment is obsolete: true
(Assignee)

Comment 54

a year ago
Created attachment 8905819 [details]
[WIP] dirty implementation: keep same stream

(In reply to Chun-Min Chang[:chunmin] from comment #51)
> The WIP patch works fine on Windows. It decodes data and creates the audio
> stream in advance in completedstate. 
> However, it fails on OSX. The first audio callback needs more than 12ms from
> calling start, no matter the stream is created in advance or not. The
> creation of audio stream on Windows takes more than 10ms, while it only
> takes 2 ~ 4 ms, so the current patch doesn't work. I think I really need to
> try keeping the same stream here.

The current WIP uses the same stream instead of switching to the pre-creating stream when looping. It stops/pauses the stream when playback finishes and then starts/resumes the stream again immediately. 

The delay is still there since the delay is caused by the gap from start to the first callback, so we should try not to stop the stream even the playback finishes. I'll try it ASAP.
(Assignee)

Updated

a year ago
Attachment #8904952 - Attachment description: [WIP] dirty implementation → [WIP] dirty implementation: Pre-create stream
(Assignee)

Comment 55

a year ago
Created attachment 8906915 [details]
[WIP] dirty implementation: Dont stop stream when looping

This patch doesn't stop the stream and keep using it when looping.
The stream is keep playing in the states loop: decoding->completed->seek->decoding-> ....

It needs to change the cubeb's behavior since cubeb will stop the audio callback if it's draining. This makes the code more complicated and harder to maintain. 

The alternative is to loop the demuxer. The end-of-stream(EOS) is detected from demuxer and it will drive the state from decoding to completed, then the seek event will be fired. If the demuxer can restart to demux from the begining when the data is demuxed to the end, then the MediaDecoderStateMachine will treat it as a endless stream. However, the position needs to be relocate and the we need to fake a seek event.

Anyway, I'll try it to see if it is more acceptable than this.
Attachment #8905819 - Attachment is obsolete: true
Yes, it's what we should be doing.
(Assignee)

Comment 57

a year ago
Created attachment 8907925 [details]
[WIP] dirty implementation: Loop decoding when demuxer returns EOS

The patch keeps decoding audio data and pushes them into the audio queue, after receiving the EOS from demuxer. However, the audiostream will treat it as an endless stream, and keep updating the playback's position. 

I tried to correct the position by the total frames of one stream calculating from the demuxed data or decoded data, but it fails[0]. I think I need to get the total frames from AudioStream instead.

[0] https://github.com/ChunMinChang/gecko-dev/commit/f038399fccada891b86ed03560d6a97ca1cfd0af
(Assignee)

Comment 58

a year ago
(In reply to Chun-Min Chang[:chunmin] from comment #57)
> Created attachment 8907925 [details]
> I tried to correct the position by the total frames of one stream
> calculating from the demuxed data or decoded data, but it fails[0]. I think
> I need to get the total frames from AudioStream instead.

The total time of the stream can be computed from the lastest item of the AudioQueue, so I use it to do modulo operation with GetClock() to correct the playback's position.
[0] https://github.com/ChunMinChang/gecko-dev/commit/fed1a4efc90f50acd634f50961190b25712ef02c
(Assignee)

Comment 59

a year ago
Created attachment 8908526 [details]
test page

1. Add event counter to test if seeking and seeked events can be received.
2. Add playbackrate setting to test playback position.
Attachment #8905345 - Attachment is obsolete: true
(Assignee)

Comment 60

a year ago
(In reply to Chun-Min Chang[:chunmin] from comment #55)
> If the demuxer can restart to demux from the begining when the data is demuxed to the end, 
> then the MediaDecoderStateMachine will treat it as a endless stream.
> However, the position needs to be relocate and the we need to fake a seek event.
To fake the events, I add MediaEventType::Loop in MediaDecoder::OnPlaybackEvent to dispatch `seeking` and `seeked` without any real seeking behavior. They will be fired when the playback position is back to beginning from the end[0].
[0] https://github.com/ChunMinChang/gecko-dev/commit/5022d408d6bef7d0a2bb8471518d723bee25e267
(Assignee)

Comment 61

a year ago
Created attachment 8910645 [details]
test page
Attachment #8904934 - Attachment is obsolete: true
Attachment #8908526 - Attachment is obsolete: true
(Assignee)

Comment 62

a year ago
Created attachment 8910647 [details] [diff] [review]
[WIP] Draft

The playback position is a bit delay.
Attachment #8904952 - Attachment is obsolete: true
Attachment #8906915 - Attachment is obsolete: true
Attachment #8907925 - Attachment is obsolete: true
(Assignee)

Comment 63

a year ago
Created attachment 8911082 [details]
test page

looping test pages for audio, video, and web audio.
Attachment #8423890 - Attachment is obsolete: true
Attachment #8910645 - Attachment is obsolete: true
(Assignee)

Comment 64

a year ago
Created attachment 8912117 [details] [diff] [review]
[WIP] Draft
Attachment #8910647 - Attachment is obsolete: true
(Assignee)

Comment 65

a year ago
Created attachment 8913577 [details] [diff] [review]
[WIP] Draft
Attachment #8912117 - Attachment is obsolete: true
(Assignee)

Comment 66

a year ago
Created attachment 8914649 [details] [diff] [review]
[WIP] Draft

The current implementation cannot pass test_mediaElementAudioSourceNodeFidelity[0]. 

In this case, <isAnyBlocked, isAnyUnBlocked>[1] stays <1, 0> after reaching the end of the audio file, so `DecodedStream::NotifyOutput` and `mLastOutputTime`[2] cannot be called and updated. Therefore, the returned value from `DecodedStream::GetPosition` stays same. As a result, no timeupdate event can be fired and the test page pends until timeout since it relies on timeupdate event to run. (Before applying this implementation, <isAnyBlocked, isAnyUnBlocked>[1] will be back to <0, 1> after <1, 0> while looping.)

The difference, after applying this implementation, is that we don't call `SourceMediaStream::Finish()` in `DecodedStream::SendData()`. The audio stream is treated as an endless stream while seamless looping.

Paul, do you have any idea how to solve it? Should we change MediaStreamGraph?


[0] http://searchfox.org/mozilla-central/rev/a4702203522745baff21e519035b6c946b7d710d/dom/media/webaudio/test/test_mediaElementAudioSourceNodeFidelity.html
[1] http://searchfox.org/mozilla-central/rev/a4702203522745baff21e519035b6c946b7d710d/dom/media/MediaStreamGraph.cpp#339-340
[2] http://searchfox.org/mozilla-central/rev/a4702203522745baff21e519035b6c946b7d710d/dom/media/mediasink/DecodedStream.cpp#752
[3] http://searchfox.org/mozilla-central/rev/a4702203522745baff21e519035b6c946b7d710d/dom/media/MediaStreamGraph.h#764
[4] http://searchfox.org/mozilla-central/rev/a4702203522745baff21e519035b6c946b7d710d/dom/media/mediasink/DecodedStream.cpp#715
Attachment #8913577 - Attachment is obsolete: true
Flags: needinfo?(padenot)
I had a listen to your patch, and I have to say that in debug-nonopt (Linux), it's indeed seamless, good job!

For the MSG part, indeed your patch break `captureStream` + looping or `MediaElementAudioSourceNode` + looping. What happens is the following:
- At the beginning of an iteration of the MSG, we update all streams (`MediaStreamGraphImpl::UpdateGraph`). Part of this is asking all MediaStream whether or not they are finished.
- If they are not finished, we check if they will under-run (= won't be able to provide data for this iteration), in `MediaStreamGraphImpl::WillUnderrun`.
- Here, we don't have enough data for this iteration, because we still report an accurate track end, without wrapping around.

I believe that if you fix this `GetEnd` issue (to not report that we're under-running), it should just work.

To catch that in a debugger, break exactly at [0]: this log message is printed when you're hitting the issue.

[0]: http://searchfox.org/mozilla-central/source/dom/media/MediaStreamGraph.cpp#477
Flags: needinfo?(padenot)

Comment 68

a year ago
Btw, is <audio> seamless on other browsers? Is there an existing spec requirement (or an addition as part of this?) that looping <audio> will be required to be seamless? I'm wondering if we should be opening tickets against Chrome et al., or if they already do this seamless?
(Assignee)

Comment 69

a year ago
(In reply to Paul Adenot (:padenot) from comment #67)
> I believe that if you fix this `GetEnd` issue (to not report that we're
> under-running), it should just work.
> 
> To catch that in a debugger, break exactly at [0]: this log message is
> printed when you're hitting the issue.
> 
> [0]:
> http://searchfox.org/mozilla-central/source/dom/media/MediaStreamGraph.
> cpp#477
Thanks a lot for your hints :) 
It really helps me to find the root cause!

The problem in my implementation is that:
1. DecodedStream::SendAudio need to get all the audio data after `mNextAudioTime` by `MediaQueue::GetElementsAfter`[0].
2. In my impelementation, the audio decoding will be restarted from the begining after decoding to the end.
   Therefore, we will keep pushing the decoded data into audio queue.
3. By given time T=mNextAudioTime, the `GetElementsAfter` should get an array containing all the audio data 
   whose time is larger than T. Its first index is computed by finding one data whose time is smaller than T[1].
4. The audio data, whose time from 0 to k, will be pushed after the ending audio data while looping, 
   and k might smaller than T=mNextAudioTime, so we will get only 1 element in this case.


Expected result:

| time < T | time => T |
+------+---+-----------+
| .... | x | ......... |
+------+---+-----------+
       |<--    A    -->|

Actual result: (while looping)

| time < T | time => T |     time < T    |
+------+---+-----------+---+---+-----+---+
| .... | x | ......... | 0 | 1 | ... | k |
+------+---+-----------+---+---+-----+---+
                                     | A |

5. Thus, we cannot get the correct `mAudioFramesWritten`[2] since it's calculated by the audio data
   returned from `GetElementsAfter`.
6. As a result, the `endPosition` of the track and the returned value from `StreamTracks::GetEnd()` will be wrong[3,4,5,6,7].

I think the easiest solution is to add the `offset` to the time of the decoded audio when we start pushing the data into the audio queue from the begining, where `offset = loopingCount * totalTimeOfTheAudio`.


[0] http://searchfox.org/mozilla-central/rev/7ba03cebf1dcf3b4c40c85a4efad42a5022cf1ec/dom/media/mediasink/DecodedStream.cpp#521
[1] http://searchfox.org/mozilla-central/rev/8efd128b48cdae3a6c6862795ce618aa1ca1c0b4/dom/media/MediaQueue.h#117
[2] http://searchfox.org/mozilla-central/rev/7ba03cebf1dcf3b4c40c85a4efad42a5022cf1ec/dom/media/mediasink/DecodedStream.cpp#498
[3] http://searchfox.org/mozilla-central/rev/7ba03cebf1dcf3b4c40c85a4efad42a5022cf1ec/dom/media/mediasink/DecodedStream.cpp#674-676,686
[4] http://searchfox.org/mozilla-central/rev/8efd128b48cdae3a6c6862795ce618aa1ca1c0b4/dom/media/MediaStreamGraph.cpp#3053
[5] http://searchfox.org/mozilla-central/rev/8efd128b48cdae3a6c6862795ce618aa1ca1c0b4/dom/media/MediaStreamGraph.cpp#308
[6] http://searchfox.org/mozilla-central/rev/8efd128b48cdae3a6c6862795ce618aa1ca1c0b4/dom/media/StreamTracks.h#224
[7] http://searchfox.org/mozilla-central/rev/8efd128b48cdae3a6c6862795ce618aa1ca1c0b4/dom/media/StreamTracks.cpp#35
(Assignee)

Comment 70

a year ago
(In reply to Jukka Jylänki from comment #68)
> Btw, is <audio> seamless on other browsers? Is there an existing spec
> requirement (or an addition as part of this?) that looping <audio> will be
> required to be seamless? I'm wondering if we should be opening tickets
> against Chrome et al., or if they already do this seamless?
AFAIK, there is no spec for seamless looping now. Chrome doesn't has seamless looping for now(but I don't know whether thet have internal discussion for this), while Edge has this feature.
(Assignee)

Comment 71

a year ago
Created attachment 8915864 [details] [diff] [review]
[WIP] Draft
Attachment #8914649 - Attachment is obsolete: true
Chun-Min, your solution with the offset sounds good, does it work fine ?
(Assignee)

Comment 73

a year ago
Created attachment 8917267 [details] [diff] [review]
[WIP] Draft
Attachment #8915864 - Attachment is obsolete: true
(Assignee)

Comment 74

a year ago
(In reply to Paul Adenot (:padenot) from comment #72)
> Chun-Min, your solution with the offset sounds good, does it work fine ?
Yes, I think it works. The test is passed on my laptop and tryserver! Thanks again for your hints!
(Assignee)

Comment 75

a year ago
Comment on attachment 8917267 [details] [diff] [review]
[WIP] Draft

Hi JW,
Could you have a look? I would like to ask your opinion since I am not sure if it's a good idea to change the behavior of MDSM and MediaQueue for looping. 
Thanks!
Attachment #8917267 - Flags: feedback?(jwwang)
Please use review board which provides more context to the code changes.
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Attachment #8917267 - Flags: feedback?(jwwang)

Comment 78

a year ago
mozreview-review
Comment on attachment 8918776 [details]
Bug 654787 - part1: Add pref for audio seamless looping;

https://reviewboard.mozilla.org/r/189632/#review195312

1. I don't like the idea to add playback/loop related logic to MediaQueue which should be a simple storage for media data.
2. The second operand of the modulo operator should be a scalar instead of a TimeUnit. See the '/' operator.
3. AudioSink expects timestamps of audio data to be mono-increasing for the gap filling algorithm to work. However, this assumption is broken when looping is introduced.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 82

a year ago
mozreview-review
Comment on attachment 8920949 [details]
Bug 654787 - part3: Use OnAudioDataRequest{Completed, Failed} in ReaderProxy;

https://reviewboard.mozilla.org/r/191910/#review197096


C/C++ static analysis found 2 defects in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`

If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: dom/media/MediaDecoderStateMachine.cpp:873
(Diff revision 1)
> +  // Flush all the data that is over the end of the song.
> +  void FlushLoopingData() {
> +    MOZ_ASSERT(ShouldFinishAudioLooping());
> +    uint64_t left = StreamSize() - PoppedSize();
> +    while (AudioQueue().GetSize() > left) {
> +      RefPtr<AudioData> x = AudioQueue().Pop();

Error: Unused "kungfudeathgrip" 'refptr<mozilla::audiodata>' objects constructed from temporary values are prohibited [clang-tidy: mozilla-kungfu-death-grip]

::: dom/media/MediaDecoderStateMachine.cpp:873
(Diff revision 1)
> +  // Flush all the data that is over the end of the song.
> +  void FlushLoopingData() {
> +    MOZ_ASSERT(ShouldFinishAudioLooping());
> +    uint64_t left = StreamSize() - PoppedSize();
> +    while (AudioQueue().GetSize() > left) {
> +      RefPtr<AudioData> x = AudioQueue().Pop();

Error: Unused "kungfudeathgrip" 'refptr<mozilla::audiodata>' objects constructed from temporary values are prohibited [clang-tidy: mozilla-kungfu-death-grip]

Comment 83

a year ago
mozreview-review
Comment on attachment 8920949 [details]
Bug 654787 - part3: Use OnAudioDataRequest{Completed, Failed} in ReaderProxy;

https://reviewboard.mozilla.org/r/191910/#review198378

Some suggestions:
1. ReaderProxy is where we ajust timestampes for audio/video samples. We can teach it about audio looping so it can adjust timestamps for audio samples when looping is enabled. This would simplify the logic of MDSM.
2. Don't pop samples when looping is off, because it coudl be turned on again.

::: dom/media/MediaDecoderStateMachine.cpp:619
(Diff revision 1)
>    {
> +    // Finish audio looping when looping is canceled and the queued data is
> +    // enough to be played till the end. On the other hand, if we're still
> +    // waiting for the decoded data, looping should be finished in
> +    // HandleAudioDecoded instead.
> +    if (ShouldFinishAudioLooping() && !mMaster->IsWaitingAudioData()) {

This doesn't make sense to me. Looping is a property of playback which shouldn't depend on the way we decode.

::: dom/media/MediaDecoderStateMachine.cpp:855
(Diff revision 1)
> +  {
> +    return mMaster->HasAudioLoopingBeenCancelled() && CanLoopToEnd();
> +  }
> +
> +  // Returns true when the queue size is larger then one whole song.
> +  bool HasEnoughLoopingData() const {

This doesn't make sense to me. The audio queue only stores enough data to keep playback going without underflow. There is no way for the audio queue to store more data than the whole stream.

::: dom/media/MediaDecoderStateMachine.cpp:2387
(Diff revision 1)
> +DecodingState::FinishAudioLooping()
> +{
> +  MOZ_ASSERT(ShouldFinishAudioLooping());
> +  // Ignore the callback for decoded data.
> +  mMaster->mAudioDataRequest.DisconnectIfExists();
> +  FlushLoopingData();

It is possible to enable looping anytime soon in the future. I don't think it is a good idea to discard the data since popped data is lost forever.

::: dom/media/MediaDecoderStateMachine.cpp:2398
(Diff revision 1)
>  DecodingState::HandleEndOfAudio()
>  {
> +  // For audio seamless looping, we restart demuxing from the beginning of the
> +  // audio and continue pushing the decoded data into the audio queue as it's a
> +  // endless stream.
> +  if (mMaster->OnAudioLooping()) {

This is wrong because EOS could be received during buffering not only in the decoding state.

::: dom/media/MediaDecoderStateMachine.cpp:2907
(Diff revision 1)
> +      aSample->mTime += mPeriod * mLoopingCount;
> +    // If the audio has not been looped yet, calculate the total number of data
> +    // and the whole duration of this audio.
> +    } else {
> +      ++mStreamSize;
> +      int64_t duration = 1000000 * aSample->mFrames / aSample->mRate;

The duration of the whole audio stream should be the end time of the last audio sample to be consistent with the definition of AudioSink::GetEndTime().
Attachment #8920949 - Flags: review?(jwwang) → review-
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 102

a year ago
mozreview-review
Comment on attachment 8924888 [details]
Bug 654787 - part5: Add the looping-offset time to audio data;

https://reviewboard.mozilla.org/r/196132/#review201332


C/C++ static analysis found 2 defects in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`

If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: dom/media/ReaderProxy.cpp:106
(Diff revision 1)
> +      aError.Code() == NS_ERROR_DOM_MEDIA_END_OF_STREAM) {
> +    ResetDecode(TrackInfo::kAudioTrack);
> +    Seek(SeekTarget(media::TimeUnit(), SeekTarget::Accurate))
> +      ->Then(mOwnerThread, __func__,
> +             [this] (const media::TimeUnit& aUnit) {
> +               mSeekRequest.Complete();

Error: Refcounted variable 'this' of type 'mozilla::readerproxy' cannot be captured by a lambda [clang-tidy: mozilla-refcounted-inside-lambda]

::: dom/media/ReaderProxy.cpp:118
(Diff revision 1)
> +                        &ReaderProxy::OnFirstLoopingAudioDataRequestCompleted,
> +                        &ReaderProxy::OnFirstLoopingAudioDataRequestFailed)
> +                 ->Track(mAudioDataRequest);
> +             },
> +             [this, aError] (const SeekRejectValue& aReject) {
> +               mSeekRequest.Complete();

Error: Refcounted variable 'this' of type 'mozilla::readerproxy' cannot be captured by a lambda [clang-tidy: mozilla-refcounted-inside-lambda]

Comment 103

a year ago
mozreview-review
Comment on attachment 8924888 [details]
Bug 654787 - part5: Add the looping-offset time to audio data;

https://reviewboard.mozilla.org/r/196132/#review201344


C/C++ static analysis found 2 defects in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`

If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: dom/media/ReaderProxy.cpp:106
(Diff revision 2)
> +      aError.Code() == NS_ERROR_DOM_MEDIA_END_OF_STREAM) {
> +    ResetDecode(TrackInfo::kAudioTrack);
> +    Seek(SeekTarget(media::TimeUnit(), SeekTarget::Accurate))
> +      ->Then(mOwnerThread, __func__,
> +             [this] (const media::TimeUnit& aUnit) {
> +               mSeekRequest.Complete();

Error: Refcounted variable 'this' of type 'mozilla::readerproxy' cannot be captured by a lambda [clang-tidy: mozilla-refcounted-inside-lambda]

::: dom/media/ReaderProxy.cpp:118
(Diff revision 2)
> +                        &ReaderProxy::OnFirstLoopingAudioDataRequestCompleted,
> +                        &ReaderProxy::OnFirstLoopingAudioDataRequestFailed)
> +                 ->Track(mAudioDataRequest);
> +             },
> +             [this, aError] (const SeekRejectValue& aReject) {
> +               mSeekRequest.Complete();

Error: Refcounted variable 'this' of type 'mozilla::readerproxy' cannot be captured by a lambda [clang-tidy: mozilla-refcounted-inside-lambda]

Comment 104

a year ago
mozreview-review
Comment on attachment 8918776 [details]
Bug 654787 - part1: Add pref for audio seamless looping;

https://reviewboard.mozilla.org/r/189632/#review201728

::: dom/media/MediaPrefs.h:217
(Diff revision 4)
>    // Enable sandboxing support for cubeb
>    DECL_MEDIA_PREF("media.cubeb.sandbox",                      CubebSandbox, bool, false);
>    DECL_MEDIA_PREF("media.videocontrols.lock-video-orientation",  VideoOrientationLockEnabled, bool, false);
>  
> +  // Audio Seamless Looping
> +  DECL_MEDIA_PREF("media.audio.seamless-looping",             AudioSeamlessLooping, bool, true);

Just call it 'media.seamless-looping' since we might need to support video seamless looping in the future. It is just that our current implementation supports seamless looping for audio-only files now.
Attachment #8918776 - Flags: review?(jwwang) → review+

Comment 105

a year ago
mozreview-review
Comment on attachment 8920949 [details]
Bug 654787 - part3: Use OnAudioDataRequest{Completed, Failed} in ReaderProxy;

https://reviewboard.mozilla.org/r/191910/#review201732

::: dom/media/MediaDecoderStateMachine.cpp:3567
(Diff revision 3)
>  }
>  
> +void MediaDecoderStateMachine::LoopingChanged()
> +{
> +  MOZ_ASSERT(OnTaskQueue());
> +  mReader->UpdateAudioLooping(ShouldAudioLoop());

ShouldAudioLoop() depends on MediaPrefs::AudioSeamlessLooping() which might change during playback. However, your patch fails to detect changes in the prefs and notify ReaderProxy about the chagne.

To make things simpler, we should read MediaPrefs::AudioSeamlessLooping() just once after decoding the metadata. Then we only need to notify ReaderProxy when mLoopping is changed.

::: dom/media/ReaderProxy.h:108
(Diff revision 3)
>  
>    // Duration, mirrored from the state machine task queue.
>    Mirror<media::NullableTimeUnit> mDuration;
> +
> +  // Indicates whether we should loop the audio.
> +  bool mAudioLooping;

Call it 'mSeamlessLoopingEnabled".
Attachment #8920949 - Flags: review?(jwwang) → review-

Comment 106

a year ago
mozreview-review
Comment on attachment 8920949 [details]
Bug 654787 - part3: Use OnAudioDataRequest{Completed, Failed} in ReaderProxy;

https://reviewboard.mozilla.org/r/191910/#review201734

::: dom/media/ReaderProxy.cpp:210
(Diff revision 3)
>  }
>  
>  void
> +ReaderProxy::UpdateAudioLooping(bool aLooping)
> +{
> +  mAudioLooping = aLooping;

Add MOZ_ASSERT(mReader->OwnerThread()->IsCurrentThreadIn()).

Comment 107

a year ago
mozreview-review
Comment on attachment 8924887 [details]
Bug 654787 - part4: Keep decoding to MDSM in ReaderProxy when looping is on;

https://reviewboard.mozilla.org/r/196130/#review201748

::: dom/media/ReaderProxy.h:95
(Diff revision 2)
>    ~ReaderProxy();
>    RefPtr<MetadataPromise> OnMetadataRead(MetadataHolder&& aMetadata);
>    RefPtr<MetadataPromise> OnMetadataNotRead(const MediaResult& aError);
>    void UpdateDuration();
>  
> +  RefPtr<ReaderProxy::AudioDataPromise>

`mach clang-format` to fix the format.

::: dom/media/ReaderProxy.cpp:60
(Diff revision 2)
>  
>  RefPtr<ReaderProxy::AudioDataPromise>
> -ReaderProxy::RequestAudioData()
> +ReaderProxy::OnAudioDataRequestCompleted(RefPtr<AudioData> aAudio)
>  {
>    MOZ_ASSERT(mOwnerThread->IsCurrentThreadIn());
>    MOZ_ASSERT(!mShutdown);

Remove this assertion for we can't guarantee this will always be called before shutdown.

::: dom/media/ReaderProxy.cpp:71
(Diff revision 2)
> +
> +RefPtr<ReaderProxy::AudioDataPromise>
> +ReaderProxy::OnAudioDataRequestFailed(const MediaResult& aError)
> +{
> +  MOZ_ASSERT(mOwnerThread->IsCurrentThreadIn());
> +  MOZ_ASSERT(!mShutdown);

Ditto.
Attachment #8924887 - Flags: review?(jwwang) → review-

Comment 108

a year ago
mozreview-review
Comment on attachment 8924888 [details]
Bug 654787 - part5: Add the looping-offset time to audio data;

https://reviewboard.mozilla.org/r/196132/#review201754

::: dom/media/ReaderProxy.cpp:61
(Diff revision 2)
> +void
> +ReaderProxy::OnFirstLoopingAudioDataRequestCompleted(RefPtr<AudioData> aAudio)
> +{
> +  MOZ_ASSERT(mOwnerThread->IsCurrentThreadIn());
> +  MOZ_ASSERT(!mShutdown);
> +  MOZ_ASSERT(!mAudioDataPromise.IsEmpty());

mAudioDataRequest.Complete() will assert !IsEmpty().

::: dom/media/ReaderProxy.cpp:99
(Diff revision 2)
>  
> +  // For audio seamless looping, the demuxer is sought to the beginning
> +  // in advance when receiving EOS. Then the demuxed data will be decoded
> +  // and send to the MDSM, so MDSM will not be aware of the looping and keep
> +  // receiving decoded audio data as usual.
> +  if (mAudioLooping &&

The code can be made simpler by using promise chaining.

::: dom/media/ReaderProxy.cpp:100
(Diff revision 2)
> +  // For audio seamless looping, the demuxer is sought to the beginning
> +  // in advance when receiving EOS. Then the demuxed data will be decoded
> +  // and send to the MDSM, so MDSM will not be aware of the looping and keep
> +  // receiving decoded audio data as usual.
> +  if (mAudioLooping &&
> +      mAudioDataPromise.IsEmpty() &&

We should assert mAudioDataPromise.IsEmpty() here, right?
Attachment #8924888 - Flags: review?(jwwang) → review-

Comment 109

a year ago
mozreview-review
Comment on attachment 8924889 [details]
Bug 654787 - part6: Correct the playback position while looping;

https://reviewboard.mozilla.org/r/196134/#review201756

::: dom/media/ReaderProxy.h:127
(Diff revision 2)
>  
>    MozPromiseHolder<AudioDataPromise> mAudioDataPromise;
>    MozPromiseRequestHolder<SeekPromise> mSeekRequest;
>    MozPromiseRequestHolder<AudioDataPromise> mAudioDataRequest;
>  
> +  // The total playing time of audio looping of previous rounds.

Reset this offset when Seek() is called.

::: dom/media/ReaderProxy.h:130
(Diff revision 2)
>    MozPromiseRequestHolder<AudioDataPromise> mAudioDataRequest;
>  
> +  // The total playing time of audio looping of previous rounds.
> +  media::TimeUnit mLoopingOffset = media::TimeUnit::Zero();
> +  // Keep tracking the latest time of decoded audio data.
> +  media::TimeUnit mLastTime = media::TimeUnit::Zero();

Ditto.

And call this member 'mLastAudioEndTime' since we might support video seamless looping in the future.

::: dom/media/ReaderProxy.cpp:114
(Diff revision 2)
>                 mSeekRequest.Complete();
>                 MOZ_ASSERT(!mAudioDataRequest.Exists());
> +               // The time of audio in audio queue is assumed to be increased
> +               // linearly, so we need to add the last ending time as the
> +               // offset to correct the audio time for next round.
> +               mLoopingOffset = mLastTime;

We should set this member when EOS is received before seeking.
Attachment #8924889 - Flags: review?(jwwang) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 119

a year ago
mozreview-review
Comment on attachment 8924888 [details]
Bug 654787 - part5: Add the looping-offset time to audio data;

https://reviewboard.mozilla.org/r/196132/#review202154


C/C++ static analysis found 4 defects in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`

If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: dom/media/ReaderProxy.cpp:83
(Diff revision 3)
> +    ResetDecode(TrackInfo::kAudioTrack);
> +    Seek(SeekTarget(media::TimeUnit(), SeekTarget::Accurate))
> +      ->Then(mOwnerThread,
> +             __func__,
> +             [this](const media::TimeUnit& aUnit) {
> +               mSeekRequest.Complete();

Error: Refcounted variable 'this' of type 'mozilla::readerproxy' cannot be captured by a lambda [clang-tidy: mozilla-refcounted-inside-lambda]

::: dom/media/ReaderProxy.cpp:93
(Diff revision 3)
> +                           __func__,
> +                           &MediaFormatReader::RequestAudioData)
> +                 ->Then(mOwnerThread,
> +                        __func__,
> +                        [this](RefPtr<AudioData> aAudio) {
> +                          mAudioDataRequest.Complete();

Error: Refcounted variable 'this' of type 'mozilla::readerproxy' cannot be captured by a lambda [clang-tidy: mozilla-refcounted-inside-lambda]

::: dom/media/ReaderProxy.cpp:97
(Diff revision 3)
> +                        [this](RefPtr<AudioData> aAudio) {
> +                          mAudioDataRequest.Complete();
> +                          mAudioDataPromise.Resolve(aAudio.forget(), __func__);
> +                        },
> +                        [this](const MediaResult& aError) {
> +                          mAudioDataRequest.Complete();

Error: Refcounted variable 'this' of type 'mozilla::readerproxy' cannot be captured by a lambda [clang-tidy: mozilla-refcounted-inside-lambda]

::: dom/media/ReaderProxy.cpp:103
(Diff revision 3)
> +                          mAudioDataPromise.Reject(aError, __func__);
> +                        })
> +                 ->Track(mAudioDataRequest);
> +             },
> +             [this, aError](const SeekRejectValue& aReject) {
> +               mSeekRequest.Complete();

Error: Refcounted variable 'this' of type 'mozilla::readerproxy' cannot be captured by a lambda [clang-tidy: mozilla-refcounted-inside-lambda]

Comment 120

a year ago
mozreview-review
Comment on attachment 8918776 [details]
Bug 654787 - part1: Add pref for audio seamless looping;

https://reviewboard.mozilla.org/r/189632/#review202678

::: modules/libpref/init/all.js:454
(Diff revision 5)
>  pref("media.suspend-bkgnd-video.delay-ms", 10000);
>  // Resume video decoding when the cursor is hovering on a background tab to
>  // reduce the resume latency and improve the user experience.
>  pref("media.resume-bkgnd-video-on-tabhover", true);;
>  
> +// Whether the media is looped seamlessly.

'Whether to enable media seamless looping.'

Comment 121

a year ago
mozreview-review
Comment on attachment 8920948 [details]
Bug 654787 - part2: Teach ReaderProxy about audio looping;

https://reviewboard.mozilla.org/r/191908/#review202682

::: dom/media/MediaDecoderStateMachine.h:341
(Diff revision 4)
>    void ResetDecode(TrackSet aTracks = TrackSet(TrackInfo::kAudioTrack,
>                                                 TrackInfo::kVideoTrack));
>  
>    void SetVideoDecodeModeInternal(VideoDecodeMode aMode);
>  
> +  bool IsAudioOnly() const { return HasAudio() && !HasVideo(); }

Merge this patch with others since it is unclear where and how these helpers are used.
Attachment #8920948 - Flags: review?(jwwang) → review-

Comment 122

a year ago
mozreview-review
Comment on attachment 8920949 [details]
Bug 654787 - part3: Use OnAudioDataRequest{Completed, Failed} in ReaderProxy;

https://reviewboard.mozilla.org/r/191910/#review202684

::: dom/media/MediaDecoderStateMachine.cpp:2204
(Diff revision 4)
>  
> +  // Check whether the media meets our policy for seamless looping.
> +  // (Before checking the media is audio only, we need to get metadata first.)
> +  mMaster->mSeamlessLoopingAllowed =
> +    MediaPrefs::SeamlessLooping() && mMaster->IsAudioOnly();
> +  Reader()->UpdateSeamlessLooping(mMaster->ShouldSeamlesslyLoop());

Just call LoopingChanged() here to ensure the change is propagated to the ready proxy if necessary.

::: dom/media/ReaderProxy.h:87
(Diff revision 4)
>    void SetVideoBlankDecode(bool aIsBlankDecode);
>  
>    void SetCanonicalDuration(
>      AbstractCanonical<media::NullableTimeUnit>* aCanonical);
>  
> +  void UpdateSeamlessLooping(bool aLooping);

'SetSeamlessLoopingEnabled(bool aEnabled)'

::: dom/media/ReaderProxy.cpp:210
(Diff revision 4)
>  }
>  
>  void
> +ReaderProxy::UpdateSeamlessLooping(bool aLooping)
> +{
> +  mSeamlessLoopingEnabled = aLooping;

Assert the current thread.
Attachment #8920949 - Flags: review?(jwwang) → review+

Comment 123

a year ago
mozreview-review
Comment on attachment 8924887 [details]
Bug 654787 - part4: Keep decoding to MDSM in ReaderProxy when looping is on;

https://reviewboard.mozilla.org/r/196130/#review202688
Attachment #8924887 - Flags: review?(jwwang) → review+

Comment 124

a year ago
mozreview-review
Comment on attachment 8924888 [details]
Bug 654787 - part5: Add the looping-offset time to audio data;

https://reviewboard.mozilla.org/r/196132/#review202696

::: dom/media/ReaderProxy.cpp:72
(Diff revision 3)
>  ReaderProxy::OnAudioDataRequestFailed(const MediaResult& aError)
>  {
>    MOZ_ASSERT(mOwnerThread->IsCurrentThreadIn());
> +  MOZ_ASSERT(mAudioDataPromise.IsEmpty());
>  
> +  // For seamless looping, the demuxer is sought to the beginning and then

It is more indentation friendly to return early if |!mSeamlessLoopingEnabled || aError.Code() != NS_ERROR_DOM_MEDIA_END_OF_STREAM|.

::: dom/media/ReaderProxy.cpp:78
(Diff revision 3)
> +  // keep requesting decoded data in advance, upon receiving EOS.
> +  // The MDSM will not be aware of the EOS and keep receiving decoded data
> +  // as usual while looping is on.
> +  if (mSeamlessLoopingEnabled &&
> +      aError.Code() == NS_ERROR_DOM_MEDIA_END_OF_STREAM) {
> +    ResetDecode(TrackInfo::kAudioTrack);

The code can be simplified by using promise chaining so you can get rid of mAudioDataPromise and its friends:

  int64_t startTime = StartTime().ToMicroseconds();
  RefPtr<MediaFormatReader> reader = mReader;
  ResetDecode(TrackInfo::kAudioTrack);
  return Seek(SeekTarget(media::TimeUnit::Zero(), SeekTarget::Accurate))
    ->Then(mReader->OwnerThread(),
           __func__,
           [reader]() { return reader->RequestAudioData(); },
           [](const SeekRejectValue& aReject) {
             return AudioDataPromise::CreateAndReject(aReject.mError, __func__);
           })
    ->Then(mOwnerThread,
           __func__,
           [startTime](RefPtr<AudioData> aAudio) {
             aAudio->AdjustForStartTime(startTime);
             return AudioDataPromise::CreateAndResolve(aAudio.forget(),
                                                       __func__);
           },
           [](const MediaResult& aError) {
             return AudioDataPromise::CreateAndReject(aError, __func__);
           });
Attachment #8924888 - Flags: review?(jwwang) → review-
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Attachment #8924892 - Attachment is obsolete: true
Attachment #8924892 - Flags: review?(jwwang)
(Assignee)

Updated

a year ago
Attachment #8924887 - Flags: review+ → review?(jwwang)
(Assignee)

Comment 133

a year ago
Comment on attachment 8920948 [details]
Bug 654787 - part2: Teach ReaderProxy about audio looping;

Correct the review status after changing the order of patches.
Attachment #8920948 - Flags: review?(jwwang) → review+
(Assignee)

Comment 134

a year ago
Comment on attachment 8924888 [details]
Bug 654787 - part5: Add the looping-offset time to audio data;

Correct the review status after changing the order of patches.
Attachment #8924888 - Flags: review?(jwwang) → review+
(Assignee)

Comment 135

a year ago
Comment on attachment 8924889 [details]
Bug 654787 - part6: Correct the playback position while looping;

Correct the review status after changing the order of patches.
Attachment #8924889 - Flags: review+ → review?(jwwang)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 140

a year ago
Comment on attachment 8924888 [details]
Bug 654787 - part5: Add the looping-offset time to audio data;

Correct the review status after changing the order of patches.
Attachment #8924888 - Flags: review?(jwwang) → review+
(Assignee)

Comment 141

a year ago
Comment on attachment 8924889 [details]
Bug 654787 - part6: Correct the playback position while looping;

Correct the review status after changing the order of patches.
Attachment #8924889 - Flags: review?(jwwang)

Comment 142

a year ago
mozreview-review
Comment on attachment 8920948 [details]
Bug 654787 - part2: Teach ReaderProxy about audio looping;

https://reviewboard.mozilla.org/r/191908/#review203172

::: dom/media/MediaDecoderStateMachine.cpp:2200
(Diff revision 5)
>    mMaster->mMetadataLoadedEvent.Notify(
>      Move(aMetadata.mInfo),
>      Move(aMetadata.mTags),
>      MediaDecoderEventVisibility::Observable);
>  
> +  // Check whether the media meets our policy for seamless looping.

wording: 'satisfy the requirement of seamless looing...'.

::: dom/media/MediaDecoderStateMachine.cpp:2203
(Diff revision 5)
>      MediaDecoderEventVisibility::Observable);
>  
> +  // Check whether the media meets our policy for seamless looping.
> +  // (Before checking the media is audio only, we need to get metadata first.)
> +  mMaster->mSeamlessLoopingAllowed =
> +    MediaPrefs::SeamlessLooping() && mMaster->IsAudioOnly();

IsAudioOnly() is used only once here. Might be worth just inlining it here.
Attachment #8920948 - Flags: review+

Comment 143

a year ago
mozreview-review
Comment on attachment 8924888 [details]
Bug 654787 - part5: Add the looping-offset time to audio data;

https://reviewboard.mozilla.org/r/196132/#review203174

::: dom/media/ReaderProxy.cpp:102
(Diff revision 5)
>               return AudioDataPromise::CreateAndReject(aReject.mError, __func__);
>             })
>      ->Then(mOwnerThread,
>             __func__,
>             [self](RefPtr<AudioData> aAudio) {
>               return self->OnAudioDataRequestCompleted(aAudio);

aAudio.forget().
Attachment #8924888 - Flags: review+

Comment 144

a year ago
mozreview-review
Comment on attachment 8924889 [details]
Bug 654787 - part6: Correct the playback position while looping;

https://reviewboard.mozilla.org/r/196134/#review203178

::: dom/media/ReaderProxy.h:90
(Diff revision 5)
>      AbstractCanonical<media::NullableTimeUnit>* aCanonical);
>  
>    void SetSeamlessLoopingEnabled(bool aEnabled);
>  
> +  // Returns the period of the stream.
> +  media::TimeUnit GetPeriod() { return mPeriod; }

We should expose a function like 'AdjustByLooping' to adjust the playback position for MDSM without exposing 2 functions.

::: dom/media/ReaderProxy.h:122
(Diff revision 5)
>  
>    // The total playing time of audio looping of previous rounds.
>    media::TimeUnit mLoopingOffset = media::TimeUnit::Zero();
>    // Keep tracking the latest time of decoded audio data.
>    media::TimeUnit mLastAudioEndTime = media::TimeUnit::Zero();
> +  // The total time of one audio stream.

// The duration of the audio track.

::: dom/media/ReaderProxy.h:123
(Diff revision 5)
>    // The total playing time of audio looping of previous rounds.
>    media::TimeUnit mLoopingOffset = media::TimeUnit::Zero();
>    // Keep tracking the latest time of decoded audio data.
>    media::TimeUnit mLastAudioEndTime = media::TimeUnit::Zero();
> +  // The total time of one audio stream.
> +  media::TimeUnit mPeriod = media::TimeUnit::Zero();

Call it 'mAudioDuration'.
Attachment #8924889 - Flags: review?(jwwang) → review-
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 152

a year ago
mozreview-review
Comment on attachment 8924889 [details]
Bug 654787 - part6: Correct the playback position while looping;

https://reviewboard.mozilla.org/r/196134/#review203664

::: dom/media/MediaDecoderStateMachine.cpp:1943
(Diff revision 6)
>  
>      if (!mSentPlaybackEndedEvent) {
>        auto clockTime =
>          std::max(mMaster->AudioEndTime(), mMaster->VideoEndTime());
> +      // Correct the time over the end once looping was turned on.
> +      if (mMaster->mSeamlessLoopingAllowed) {

We should always let ReaderProxy adjust the time since it has all the knowledge to adjust the time correctly no matter seamless looping is enabled or not.

::: dom/media/MediaDecoderStateMachine.cpp:3484
(Diff revision 6)
> -    const auto clockTime = GetClock();
> +    auto clockTime = GetClock();
> +
> +    // Once looping was turned on, the time is probably larger than the duration
> +    // of the audio track, so the time over the end should be corrected.
> +    bool loopback = false;
> +    if (mSeamlessLoopingAllowed) {

Ditto.

::: dom/media/ReaderProxy.h:120
(Diff revision 6)
>    // The total duration of audio looping in previous rounds.
>    media::TimeUnit mLoopingOffset = media::TimeUnit::Zero();
>    // To keep tracking the latest time of decoded audio data.
>    media::TimeUnit mLastAudioEndTime = media::TimeUnit::Zero();
> +  // The duration of the audio track.
> +  media::TimeUnit mAudioDuration = media::TimeUnit::Zero();

Init the value to be TimeUnit::Invalid() which is a good sentinel to know whether we have a valid audio duration or not.

::: dom/media/ReaderProxy.cpp:303
(Diff revision 6)
> +  MOZ_ASSERT(mOwnerThread->IsCurrentThreadIn());
> +  MOZ_ASSERT(!mShutdown);
> +  MOZ_ASSERT(!mSeamlessLoopingBlocked);
> +  // We don't check mSeamlessLoopingEnabled here since we still need to
> +  // correct the time even the mSeamlessLoopingEnabled is off.
> +  if (mAudioDuration <= media::TimeUnit::Zero()) {

Check mAudioDuration.IsPositive() for adjustment is needed when we have a positive audio duration.
Attachment #8924889 - Flags: review?(jwwang) → review+

Comment 153

a year ago
mozreview-review
Comment on attachment 8924890 [details]
Bug 654787 - part7: Stop playing and decoding when looping is cancelled;

https://reviewboard.mozilla.org/r/196136/#review203672

::: dom/media/MediaDecoderStateMachine.cpp:3502
(Diff revision 6)
>        mReader->AdjustByLooping(clockTime);
>        if (clockTime < GetMediaTime()) {
> +        if (mLooping) {
> -        loopback = true;
> +          loopback = true;
> +        } else { // Stop playing and decoding if looping is cancelled.
> +          StopPlayback();

This code should move to DecodingState::Step() so DecodingState can check whether to go to CompletedState if looping is off.
Attachment #8924890 - Flags: review?(jwwang) → review-
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 157

a year ago
Comment on attachment 8924887 [details]
Bug 654787 - part4: Keep decoding to MDSM in ReaderProxy when looping is on;

This patch misses the review after rearranging the patch order.
Attachment #8924887 - Flags: review?(jwwang)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 160

a year ago
mozreview-review
Comment on attachment 8924890 [details]
Bug 654787 - part7: Stop playing and decoding when looping is cancelled;

https://reviewboard.mozilla.org/r/196136/#review205202

::: dom/media/MediaDecoderStateMachine.cpp:630
(Diff revision 8)
> +    auto before = mMaster->GetMediaTime();
>      mMaster->UpdatePlaybackPositionPeriodically();
>  
> +    // After looping is cancelled, the time won't be corrected.
> +    // If the time is over the end of the media track, then we need to stop it.
> +    if (before <= mMaster->GetMediaTime()) {

Why this check?

::: dom/media/MediaDecoderStateMachine.cpp:638
(Diff revision 8)
> +      if (adjusted < before) {
> +        mMaster->StopPlayback();
> +        mMaster->mAudioDataRequest.DisconnectIfExists();
> +        AudioQueue().Finish();
> +        mMaster->mAudioCompleted = true;
> +        GoToCompletedState();

Just call SetState<CompletedState>().

::: dom/media/MediaDecoderStateMachine.cpp:3508
(Diff revision 8)
>  
>      // Once looping was turned on, the time is probably larger than the duration
>      // of the media track, so the time over the end should be corrected.
>      bool loopback = false;
>      mReader->AdjustByLooping(clockTime);
> -    if (clockTime < GetMediaTime()) {
> +    if (clockTime < GetMediaTime() && mLooping) {

loopback = clockTime < GetMediaTime() && mLooping;
Attachment #8924890 - Flags: review?(jwwang)
(Assignee)

Comment 161

a year ago
mozreview-review-reply
Comment on attachment 8924890 [details]
Bug 654787 - part7: Stop playing and decoding when looping is cancelled;

https://reviewboard.mozilla.org/r/196136/#review205202

> Why this check?

Without the check, ```adjusted < before``` will be true in two cases, 1) audio is looped 2) audio is over the end, after applying ```AdjustByLooping```. This is used to distinguish which case it is. The ```before <= mMaster->GetMediaTime()``` will be false when audio is looped.

> Just call SetState<CompletedState>().

```CompletedState``` is defined after ```DecodingState::Step()```, so we need to move the whole ```DecodingState::Step()``` to somewhere after ```MediaDecoderStateMachine::CompletedState``` to achieve this.

Comment 162

a year ago
mozreview-review
Comment on attachment 8924890 [details]
Bug 654787 - part7: Stop playing and decoding when looping is cancelled;

https://reviewboard.mozilla.org/r/196136/#review206278

::: dom/media/MediaDecoderStateMachine.cpp:630
(Diff revision 8)
> +    auto before = mMaster->GetMediaTime();
>      mMaster->UpdatePlaybackPositionPeriodically();
>  
> +    // After looping is cancelled, the time won't be corrected.
> +    // If the time is over the end of the media track, then we need to stop it.
> +    if (before <= mMaster->GetMediaTime()) {

I think it would be clearer to check:

if (adjusted < before && !mMaster->mLooping) {
  // Stop looping and go to CompletedState.
}

Comment 163

a year ago
mozreview-review
Comment on attachment 8924891 [details]
Bug 654787 - part8: Fire seeking and seeked events when looping back to the beginning;

https://reviewboard.mozilla.org/r/196138/#review206282
Attachment #8924891 - Flags: review?(jwwang) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 174

a year ago
mozreview-review
Comment on attachment 8924890 [details]
Bug 654787 - part7: Stop playing and decoding when looping is cancelled;

https://reviewboard.mozilla.org/r/196136/#review206806
Attachment #8924890 - Flags: review?(jwwang) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 177

a year ago
Comment on attachment 8924887 [details]
Bug 654787 - part4: Keep decoding to MDSM in ReaderProxy when looping is on;

JW, could you take a look? this patch is missed review due to reordering. Thanks!
Attachment #8924887 - Flags: review?(jwwang)
(In reply to Chun-Min Chang[:chunmin] from comment #177)
> Comment on attachment 8924887 [details]
> Bug 654787 - part4: Keep decoding to MDSM in ReaderProxy when looping is on;
> 
> JW, could you take a look? this patch is missed review due to reordering.
> Thanks!

LGTM.
Attachment #8924887 - Flags: review?(jwwang) → review+
All the patches got r+! \o/
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Keywords: checkin-needed
The open issues from part 3 and 5 have set to fixed in MozReview so that these changes can land, else reviewboard blocks this. Please update these issues and request checkin-needed again. Thank you.
Flags: needinfo?(cchang)
Keywords: checkin-needed
(Assignee)

Comment 186

a year ago
(In reply to Sebastian Hengst [:aryx][:archaeopteryx] (needinfo on intermittent or backout) from comment #185)
> The open issues from part 3 and 5 have set to fixed in MozReview so that
> these changes can land, else reviewboard blocks this. Please update these
> issues and request checkin-needed again. Thank you.
Thanks for your reminder!
Flags: needinfo?(cchang)
Keywords: checkin-needed

Comment 187

a year ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/8f07305a2f61
part1: Add pref for audio seamless looping; r=jwwang
https://hg.mozilla.org/integration/autoland/rev/56da2178db82
part2: Teach ReaderProxy about audio looping; r=jwwang
https://hg.mozilla.org/integration/autoland/rev/f47b6f664619
part3: Use OnAudioDataRequest{Completed, Failed} in ReaderProxy; r=jwwang
https://hg.mozilla.org/integration/autoland/rev/3e03dd51495c
part4: Keep decoding to MDSM in ReaderProxy when looping is on; r=jwwang
https://hg.mozilla.org/integration/autoland/rev/a37aa03a3a02
part5: Add the looping-offset time to audio data; r=jwwang
https://hg.mozilla.org/integration/autoland/rev/453339edb639
part6: Correct the playback position while looping; r=jwwang
https://hg.mozilla.org/integration/autoland/rev/fa72cd9ce72d
part7: Stop playing and decoding when looping is cancelled; r=jwwang
https://hg.mozilla.org/integration/autoland/rev/c75b2dcf8c6e
part8: Fire seeking and seeked events when looping back to the beginning; r=jwwang
Keywords: checkin-needed
(Assignee)

Updated

a year ago
Depends on: 1421505
Depends on: 1487797
(Assignee)

Updated

a month ago
Depends on: 1496474
(Assignee)

Updated

a month ago
Blocks: 1498733
You need to log in before you can comment on or make changes to this bug.