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.

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.

Reopening this to track seamless looping. Bug 449157 now tracks basic (non-seamless) looping support.

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.

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?

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?

Thanks, I downloaded the file, but I wonder if it has the wrong file? It did not contain a file loop.html.

Oops, went too fast, this has the right files.

Perfect, that workaround is working great! Thanks Paul!

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.

Moving this to backlog. This is too late to take in 1.4

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.

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.

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

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

ChunMin, Per discussion, please put this on your plate.

Sure.

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.

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.

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.

(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

Keep the same logs but adding the test-page

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.

(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

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

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

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

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.

We really need to keep the same cubeb stream here, it will be more reliable. JW, how hard would this be ?

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.

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

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?

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.

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

Good, then I don't need to care the bound of overflow if I want to keep using the same stream.

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.

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

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

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

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.

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

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.

Yes, it's what we should be doing.

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

(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

1. Add event counter to test if seeking and seeked events can be received. 2. Add playbackrate setting to test playback position.

(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

The playback position is a bit delay.

looping test pages for audio, video, and web audio.

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

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

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?

(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

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

Chun-Min, your solution with the offset sounds good, does it work fine ?

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

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!

Please use review board which provides more context to the code changes.

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

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 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 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.

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".

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 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.

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?

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.

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 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 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.

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.

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

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__); });

Comment on attachment 8920948 [details] Bug 654787 - part2: Teach ReaderProxy about audio looping; Correct the review status after changing the order of patches.

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.

Comment on attachment 8924889 [details] Bug 654787 - part6: Correct the playback position while looping; Correct the review status after changing the order of patches.

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.

Comment on attachment 8924889 [details] Bug 654787 - part6: Correct the playback position while looping; Correct the review status after changing the order of patches.

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.

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

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'.

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.

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.

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.

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;

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

Comment on attachment 8924890 [details] Bug 654787 - part7: Stop playing and decoding when looping is cancelled; https://reviewboard.mozilla.org/r/196136/#review206806