Per Bug 1146795 Comment 30, Bug 1146795 Comment 32, let AudioSink / RawAudioSink subclass AudioBaseSink, and MDSM could dynamically create corresponding sink according to metadata. The implementation of RawAudioSink will be separated to another bug

Provide a capability of creating different type of AudioSink by abstracting basic functional APIs to AudioBaseSink.

I would prefer the names: AudioSink <-- top level abstract class for audio consuming PlainAudioSink <-- consumer of decoded audio EncodedAudioSink <-- consumer of encoded audio (not encrypted) EncryptedAudioSink <-- consumer of encrypted audio which can do decryption or decoding and delegate most of its job to EncodedAudioSink or PlainAudioSink. This class hierarchy will improve reusability. Thoughts?

I think by introducing class hierarchy (Comment 2) is quite clear to understand what responsibility the Sink has. And I draw a data flow according to the class hierarchy. (as attached) By this design, we might need to create *at-least-one* AudioSink for each playback even it's a clear content (one EncodedAudioSink, one plainAudioSink). With this design, we could target to handle different cases of mixing decrypt/decode/render stages against both EME / non-EME situations, in which would be useful for non-protected content playback on TV. But I think MDSM must split current requesting data procedure into 2 steps (demux & decode) as a prerequisite. Does it make sense to you ?

(In reply to Kilik Kuo [:kikuo][:kilikkuo] from comment #3) > But I think MDSM must split current requesting data procedure into 2 steps > (demux & decode) as a prerequisite. Can you give some code snippet to illustrate it?

(In reply to JW Wang [:jwwang] from comment #4) > (In reply to Kilik Kuo [:kikuo][:kilikkuo] from comment #3) > > But I think MDSM must split current requesting data procedure into 2 steps > > (demux & decode) as a prerequisite. > > Can you give some code snippet to illustrate it? Thanks JW, Per discussion off-line, my fog of confusion is cleared, for simplicity and logic consistency, there's only one (SomeType)AudioSink in MDSM. In the following patches, MediaFormatReader should be capable of configuring the type of returning mediadata(Encrypted/Encoded/Plained). And MDSM could push these incoming mediadata into corresponding sinks.

Abstract original AudioSink as base class, and make PlainAudioSink / EncryptedAudioSink sub-class it.

Comment on attachment 8643441 [details] [diff] [review] Abstract original AudioSink as base class and Create PlainAudioSink/EncryptedAudioSink_255922 Hi JW, I modified the patch with new class hierarchy. Could you help review this ?

Comment on attachment 8643441 [details] [diff] [review] Abstract original AudioSink as base class and Create PlainAudioSink/EncryptedAudioSink_255922 Review of attachment 8643441 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/media/AudioSink.h @@ +43,5 @@ > + virtual nsRefPtr<GenericPromise> Init() > + { > + nsRefPtr<GenericPromise> p = mEndPromise.Ensure(__func__); > + mEndPromise.Reject(NS_ERROR_FAILURE, __func__); > + return p; The implementation doesn't make much sense. Why not make it a pure virtual? @@ +67,4 @@ > > // Wake up the audio loop if it is waiting for data to play or the audio > // queue is finished. > + virtual void NotifyData() = 0; This function will go. @@ +84,5 @@ > + GetReentrantMonitor().AssertCurrentThreadIn(); > + } > + > + MediaQueue<MediaData>& mAudioQueue; > + mutable ReentrantMonitor mMonitor; The sub-classes should have their own synchronization tools. Don't assume it in the base class. @@ +94,5 @@ > + const int64_t mStartTime; > + > + const AudioInfo mInfo; > + > + dom::AudioChannel mChannel; const. @@ +95,5 @@ > + > + const AudioInfo mInfo; > + > + dom::AudioChannel mChannel; > + MozPromiseHolder<GenericPromise> mEndPromise; This should move to the sub-classes. @@ +98,5 @@ > + dom::AudioChannel mChannel; > + MozPromiseHolder<GenericPromise> mEndPromise; > +}; > + > +class PlainAudioSink final : public AudioSink { This should go to its own file. @@ +225,5 @@ > + * rendering capability. > + * Encrypted raw media data should flow into EncryptedAudioSink after being > + * demuxed. > + */ > +class EncryptedAudioSink final : public AudioSink Ditto. ::: dom/media/MediaDecoderStateMachine.cpp @@ +1792,5 @@ > } > } > > void > +MediaDecoderStateMachine::EnsureAudioSinkSetup() What's the point in splitting StartAudioThread into 2 functions? ::: dom/media/MediaDecoderStateMachine.h @@ +509,5 @@ > // decoder monitor must be held with exactly one lock count. Called on the > // state machine thread. > void UpdateRenderedVideoFrames(); > > + // Stops the working thread in audio sink and shutdown audio sink. Don't mention about the working thread which a implementation specific detail about AudioSink. It shouldn't be exposed outside AudioSink.

Hi JW, Thanks for the review. I addressed issues in Comment 8 and re-based the code to create a new patch. (Because I need AudioSink::NotifyData() to be removed) I also create a new folder dom/media/mediasink and put AudioSink.h/PlainAudioSink.{h,cpp}/EncryptedAudioSink.{h,cpp} inside for better management. In the future, I can put VideoSink-related files inside. Issues are addressed as follows, 1. Make |nsRefPtr<GenericPromise> Init()| a pure virtual function in base class AudioSink. 2. There's no |virtual void NotifyData()| in the interface at all. 3. Remove ReentrantMonitor from base class, and put it back to PlainAudioSink. 4. Add const to member variable |dom::AudioChannel mChannel| 5. Remove "working thread" related wording from the comment. 6. The reason I split StartAudioThread() into 2 functions is that I'd like to handle all logic check in EnsureAudioSinkSetup() and make StartAudioSink() do the actual creation/start. Do you suggest keeping it as 1 function ? I'm also ok with that since right now it's just a small code snippet.

Comment on attachment 8645424 [details] [diff] [review] Abstract original AudioSink as a base class and create PlainAudioSink & EncryptedAudioSink_256893 Review of attachment 8645424 [details] [diff] [review]: ----------------------------------------------------------------- Please use hg mv to preserve the revision history of AudioSink. ::: dom/media/MediaDecoderStateMachine.cpp @@ +14,5 @@ > > #include "MediaDecoderStateMachine.h" > #include "MediaTimer.h" > +#include "mozilla/dom/AudioSink.h" > +#include "mozilla/dom/PlainAudioSink.h" It should be enough just inlcude "PlainAudioSink.h". @@ +1791,5 @@ > return; > } > > if (HasAudio() && !mAudioSink) { > + StartAudioSink(); Since StartAudioSink() is only called here, there isn't much benefit to split StartAudioThread() into 2 functions. It is also more confusing to developers to understand when to call EnsureAudioSinkSetup() or StartAudioSink(). ::: dom/media/mediasink/AudioSink.h @@ +42,5 @@ > + > + // Shut down the AudioSink's resources. > + virtual void Shutdown() = 0; > + > + // Provide subclass audio-related information The first 3 functions are used to change audio playback settings. They have nothing to do with sub-classes. @@ +46,5 @@ > + // Provide subclass audio-related information > + virtual void SetVolume(double aVolume) = 0; > + virtual void SetPlaybackRate(double aPlaybackRate) = 0; > + virtual void SetPreservesPitch(bool aPreservesPitch) = 0; > + virtual void SetPlaying(bool aPlaying) = 0; This function is used to pause/resume audio playback. ::: dom/media/mediasink/EncryptedAudioSink.cpp @@ +1,1 @@ > +/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */ I would prefer to save this file until we have a concrete implementation. The fake implementation is not very useful for now. ::: dom/media/mediasink/moz.build @@ +3,5 @@ > +# This Source Code Form is subject to the terms of the Mozilla Public > +# License, v. 2.0. If a copy of the MPL was not distributed with this > +# file, You can obtain one at http://mozilla.org/MPL/2.0/. > + > +EXPORTS.mozilla.dom += [ Ask a build peer to review this file.

(In reply to JW Wang [:jwwang] from comment #10) > Comment on attachment 8645424 [details] [diff] [review] > Abstract original AudioSink as a base class and create PlainAudioSink & > EncryptedAudioSink_256893 > > Review of attachment 8645424 [details] [diff] [review]: > ----------------------------------------------------------------- > > Please use hg mv to preserve the revision history of AudioSink. WOW, I didn't know this command, thanks for the tips, I'll try :) > > ::: dom/media/MediaDecoderStateMachine.cpp > @@ +14,5 @@ > > > > #include "MediaDecoderStateMachine.h" > > #include "MediaTimer.h" > > +#include "mozilla/dom/AudioSink.h" > > +#include "mozilla/dom/PlainAudioSink.h" > > It should be enough just inlcude "PlainAudioSink.h". Will address. > > @@ +1791,5 @@ > > return; > > } > > > > if (HasAudio() && !mAudioSink) { > > + StartAudioSink(); > > Since StartAudioSink() is only called here, there isn't much benefit to > split StartAudioThread() into 2 functions. It is also more confusing to > developers to understand when to call EnsureAudioSinkSetup() or > StartAudioSink(). Got it ! > > ::: dom/media/mediasink/AudioSink.h > @@ +42,5 @@ > > + > > + // Shut down the AudioSink's resources. > > + virtual void Shutdown() = 0; > > + > > + // Provide subclass audio-related information > > The first 3 functions are used to change audio playback settings. They have > nothing to do with sub-classes. > > @@ +46,5 @@ > > + // Provide subclass audio-related information > > + virtual void SetVolume(double aVolume) = 0; > > + virtual void SetPlaybackRate(double aPlaybackRate) = 0; > > + virtual void SetPreservesPitch(bool aPreservesPitch) = 0; > > + virtual void SetPlaying(bool aPlaying) = 0; > > This function is used to pause/resume audio playback. > > ::: dom/media/mediasink/EncryptedAudioSink.cpp > @@ +1,1 @@ > > +/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */ > > I would prefer to save this file until we have a concrete implementation. > The fake implementation is not very useful for now. I'll create these two files in Bug 1191200. Thanks for the suggestion.

Hi JW, I addressed issues in Comment 11. Need your help to review it again. Regarding dom/media/moz.build, I'll ask for cpreace's help. Thank you.

Comment on attachment 8645638 [details] [diff] [review] Abstract original AudioSink as base class and create PlainAudioSink_rv256893_v2 Hi Chris, In this patch, I moved dom/media/AudioSink.{h,cpp} to dom/media/mediasink/PlainAudioSink.{h, cpp} and create a new dom/media/mediasink/AudioSink.h as a base class. And export dom/media/mediasink/AudioSink.h & dom/media/mediasink/PlainAudioSink.h to EXPORTS.mozilla.dom. If this is ok ? or export those headers to EXPORTS.mozilla.dom.media (which I think it's a little bit lenghty)

Comment on attachment 8645638 [details] [diff] [review] Abstract original AudioSink as base class and create PlainAudioSink_rv256893_v2 Review of attachment 8645638 [details] [diff] [review]: ----------------------------------------------------------------- "Plain" is not very descriptive. Can you call it something more descriptive, like "AudioStreamSink" please? Apart from the name, and exports, it looks fine. ::: dom/media/mediasink/moz.build @@ +3,5 @@ > +# This Source Code Form is subject to the terms of the Mozilla Public > +# License, v. 2.0. If a copy of the MPL was not distributed with this > +# file, You can obtain one at http://mozilla.org/MPL/2.0/. > + > +EXPORTS.mozilla.dom += [ Why do you want to stick AudioSink in mozilla/dom? They're not exposed to web content, and they're not in the mozilla::dom namespace, so I don't see why they should be in there.

Comment on attachment 8645638 [details] [diff] [review] Abstract original AudioSink as base class and create PlainAudioSink_rv256893_v2 Review of attachment 8645638 [details] [diff] [review]: ----------------------------------------------------------------- Please split the patch so each reviewer knows which part to review. ::: dom/media/MediaDecoderStateMachine.cpp @@ +13,5 @@ > #include <stdint.h> > > #include "MediaDecoderStateMachine.h" > #include "MediaTimer.h" > +#include "mozilla/dom/PlainAudioSink.h" I don't think mozilla/dom is a good place for the header. The header should be used internally by dom/media files and not exported to the outside. ::: dom/media/mediasink/PlainAudioSink.h @@ -61,5 @@ > AUDIOSINK_STATE_SHUTDOWN, > AUDIOSINK_STATE_ERROR > }; > > - ~AudioSink() {} virtual destructor.

(In reply to Chris Pearce (:cpearce) from comment #14) > > "Plain" is not very descriptive. Can you call it something more descriptive, > like "AudioStreamSink" please? > > Apart from the name, and exports, it looks fine. > > ::: dom/media/mediasink/moz.build > @@ +3,5 @@ > > +# This Source Code Form is subject to the terms of the Mozilla Public > > +# License, v. 2.0. If a copy of the MPL was not distributed with this > > +# file, You can obtain one at http://mozilla.org/MPL/2.0/. > > + > > +EXPORTS.mozilla.dom += [ > > Why do you want to stick AudioSink in mozilla/dom? They're not exposed to > web content, and they're not in the mozilla::dom namespace, so I don't see > why they should be in there. Thanks for the review. I think making these sink classes in the mozilla::media namespace and exporting header files to mozilla.media would be nicer. I'll do that. Regarding class naming, the name PlainAudioSink came up because I'm planning that there will be an EncryptedAudioSink for encrypted data (Bug 1191200). In cryptography, "plain" and "encrypted" is used commonly to identify the text. Using AudioStreamSink is still not able to clarify what kind of data it contains. Maybe call them this way, AudioStreamSink ==> clear audio data EncodedAudioStreamSink ==> encoded audio data EncryptedAudioStreamSink ==> encrypted + encoded audio data Would that be ok ?

Do you really need to export the header? The original AudioSink.h is not exported at all.

(In reply to JW Wang [:jwwang] from comment #17) > Do you really need to export the header? The original AudioSink.h is not > exported at all. hmm...I was thinking avoid using relative path including for these headers, since I don't think it's a good way to go and it's rarely seen under dom/media, so I chose to export them to mozilla/media for MediaDecoderStateMachie.cpp(in parent folder) to include. But now I found these cases [1],[2]. These header files shouldn't be used by others except modules in dom/media. I'll remove the EXPORT script. Thanks ~ [1] https://dxr.allizom.org/mozilla-central/source/dom/media/TrackUnionStream.cpp#27 [2] https://dxr.allizom.org/mozilla-central/source/dom/media/AudioCaptureStream.cpp#18

Sorry for the ni? spam :( (In reply to Kilik Kuo [:kikuo][:kilikkuo] from comment #16) > > ::: dom/media/mediasink/moz.build > > @@ +3,5 @@ > > > +# This Source Code Form is subject to the terms of the Mozilla Public > > > +# License, v. 2.0. If a copy of the MPL was not distributed with this > > > +# file, You can obtain one at http://mozilla.org/MPL/2.0/. > > > + > > > +EXPORTS.mozilla.dom += [ > > > > Why do you want to stick AudioSink in mozilla/dom? They're not exposed to > > web content, and they're not in the mozilla::dom namespace, so I don't see > > why they should be in there. > > Thanks for the review. > I think making these sink classes in the mozilla::media namespace and > exporting header files to mozilla.media would be nicer. > I'll do that. According to Comment 18, there will be no need to use EXPORTS in dom/media/mediasink/moz.build, > > Regarding class naming, the name PlainAudioSink came up because I'm planning > that there will be an EncryptedAudioSink for encrypted data (Bug 1191200). > In cryptography, "plain" and "encrypted" is used commonly to identify the > text. Using AudioStreamSink is still not able to clarify what kind of data > it contains. > > Maybe call them this way, > > AudioStreamSink ==> clear audio data > EncodedAudioStreamSink ==> encoded audio data > EncryptedAudioStreamSink ==> encrypted + encoded audio data > > Would that be ok ? I'd like to suggest using "Data" instead of "Stream", it's more accurate to what these classes contain. AudioStreamSink ==> AudioDataSink EncodedAudioStreamSink ==> EncodedAudioDataSink EncryptedAudioStreamSink ==> EncryptedAudioDataSink Thoughts ?

You could use DecodedAudioDataSink, RawAudioDataSink, EncryptedAudioDataSink. "Raw" here means compressed but not encrypted, matching the name of MediaRawData from MediaData.h. I don't think you should be changing the policy on exporting headers piecemeal. If we are to change how we export headers, we should do everything at once, so we move from one consistent state to another. We should not be in a state where some headers are exported but not others.

(In reply to Chris Pearce (:cpearce) from comment #20) > You could use DecodedAudioDataSink, RawAudioDataSink, EncryptedAudioDataSink. > > "Raw" here means compressed but not encrypted, matching the name of > MediaRawData from MediaData.h. Got it ! > > I don't think you should be changing the policy on exporting headers > piecemeal. If we are to change how we export headers, we should do > everything at once, so we move from one consistent state to another. We > should not be in a state where some headers are exported but not others. Thanks for the reply, now I understand more about the EXPORTS keyword in moz.build :)

Addressed issues in Comment 15. Also rename to DecodedAudioDataSink based on Comment 20.

Addressed issues in Comment 14. No need to export these logically used headers and also rename the class.

Comment on attachment 8646855 [details] [diff] [review] [Part1]Abstract original AudioSink as base class and create DecodedAudioDataSink_r256893_v3 Review of attachment 8646855 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/media/mediasink/AudioSink.h @@ +5,5 @@ > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > +#if !defined(AudioSink_h__) > +#define AudioSink_h__ > + > +#include "MediaInfo.h" No need to include this header here. ::: dom/media/mediasink/DecodedAudioDataSink.h @@ +8,2 @@ > > +#include "AudioSink.h" You should also include "MediaInfo.h" because AudioInfo is referenced by DecodedAudioDataSink instead of its base class.

(In reply to JW Wang [:jwwang] from comment #24) > Comment on attachment 8646855 [details] [diff] [review] > [Part1]Abstract original AudioSink as base class and create > DecodedAudioDataSink_r256893_v3 > > Review of attachment 8646855 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/media/mediasink/AudioSink.h > @@ +5,5 @@ > > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > > +#if !defined(AudioSink_h__) > > +#define AudioSink_h__ > > + > > +#include "MediaInfo.h" > > No need to include this header here. > > ::: dom/media/mediasink/DecodedAudioDataSink.h > @@ +8,2 @@ > > > > +#include "AudioSink.h" > > You should also include "MediaInfo.h" because AudioInfo is referenced by > DecodedAudioDataSink instead of its base class. Thanks for the review :) I'll move |#include "MediaInfo.h"| from AudioSink.h to DecodedAudioDataSink.h. Based on the modification, I'll add the following class forward declaration back in AudioSink.h, they will be needed. class MediaData; template <class T> class MediaQueue;

Carry r+ from Comment 24, rebase to recent changeset and issues are addressed.

Carry r+ from cpearce and rebase to recent changeset.

try run, https://treeherder.mozilla.org/#/jobs?repo=try&revision=4bbf8ebda9de looks good !

This didn't seem to apply cleanly. Could a rebased patch be posted?

Sure, it seems some code logic around |mDecodedStream| are wrapped into functions. I'll provide a rebased patch again.

Per Comment 30, rebased a patch based on changeset 258072 and carry r+.

Per Comment 30, rebased a patch based on changeset 258072 and carry r+.

try run seems ok, https://treeherder.mozilla.org/#/jobs?repo=try&revision=c185e224b47b

Comment on attachment 8649196 [details] [diff] [review] [Part1] Abstract original AudioSink as base class and create DecodedAudioDataSink_v5_r258072 Review of attachment 8649196 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/media/MediaDecoderStateMachine.cpp @@ +2368,5 @@ > } > // Play the remaining media. We want to run AdvanceFrame() at least > // once to ensure the current playback position is advanced to the > // end of the media, and so that we update the readyState. > + MaybeStartPlayback(); I guess you made a mistake when rebasing the patch. It is wrong to move MaybeStartPlayback() out of the if statement.

Hi Kilik, Please check comment 40.

(In reply to JW Wang [:jwwang] from comment #41) > Hi Kilik, > Please check comment 40. oh no, it's really a mistake @@ I'll fix this ASAP.

(In reply to JW Wang [:jwwang] from comment #40) > Comment on attachment 8649196 [details] [diff] [review] > [Part1] Abstract original AudioSink as base class and create > DecodedAudioDataSink_v5_r258072 > > Review of attachment 8649196 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/media/MediaDecoderStateMachine.cpp > @@ +2368,5 @@ > > } > > // Play the remaining media. We want to run AdvanceFrame() at least > > // once to ensure the current playback position is advanced to the > > // end of the media, and so that we update the readyState. > > + MaybeStartPlayback(); > > I guess you made a mistake when rebasing the patch. It is wrong to move > MaybeStartPlayback() out of the if statement. Hi JW, Since the incorrect logic was already landed, and we can't back it out now. So what I need to do is to create another patch(bug?) to only move this line of code into if-statement and make it reviewed+, right ? How about the r- patch ? This is my first time encountering this situation (REOPEN), I'm not quite sure about how the procedure should go.

Submit a new patch to fix the problem.

Hi JW, please help review this correction.

(In reply to Kilik Kuo [:kikuo][:kilikkuo] from comment #46) > https://treeherder.mozilla.org/#/jobs?repo=try&revision=c9739ee9a48d Try run looks ok !

Comment on attachment 8649196 [details] [diff] [review] [Part1] Abstract original AudioSink as base class and create DecodedAudioDataSink_v5_r258072 To fix REOPEN issue, obsoleted this patch to avoid confusion

Comment on attachment 8649197 [details] [diff] [review] [Part2]Create dom/media/mediasink/moz.build and modify dom/media/moz.build according to it_v5_r258072 To fix REOPEN issue, obsoleted this patch to avoid confusion