1188268 - Abstract AudioSink as a base class and create PlainAudioSink & EncryptedAudioSink derived from it

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, defect, P1)

Description

[Kilik Kuo [:kikuo] (inactive)]

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

Comment 1

[Kilik Kuo [:kikuo] (inactive)]

Attachment: Abstract APIs from class AudioSink to AudioBaseSink class and add RawAudioSink class_255041

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

Comment 2

[JW Wang [:jwwang] [:jw_wang]]

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?

Comment 3

[Kilik Kuo [:kikuo] (inactive)]

Attachment: MediaSink Flow.svg

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 ?

Comment 4

[JW Wang [:jwwang] [:jw_wang]]

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

Comment 5

[Kilik Kuo [:kikuo] (inactive)]

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

Comment 6

[Kilik Kuo [:kikuo] (inactive)]

Attachment: Abstract original AudioSink as base class and Create PlainAudioSink/EncryptedAudioSink_255922

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

Comment 7

[Kilik Kuo [:kikuo] (inactive)]

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 8

[JW Wang [:jwwang] [:jw_wang]]

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.

Comment 9

[Kilik Kuo [:kikuo] (inactive)]

Attachment: Abstract original AudioSink as a base class and create PlainAudioSink & EncryptedAudioSink_256893

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 10

[JW Wang [:jwwang] [:jw_wang]]

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.

Comment 11

[Kilik Kuo [:kikuo] (inactive)]

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

Comment 12

[Kilik Kuo [:kikuo] (inactive)]

Attachment: Abstract original AudioSink as base class and create PlainAudioSink_rv256893_v2

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 13

[Kilik Kuo [:kikuo] (inactive)]

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 14

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

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 15

[JW Wang [:jwwang] [:jw_wang]]

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.

Comment 16

[Kilik Kuo [:kikuo] (inactive)]

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

Comment 17

[JW Wang [:jwwang] [:jw_wang]]

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

Comment 18

[Kilik Kuo [:kikuo] (inactive)]

(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

Comment 19

[Kilik Kuo [:kikuo] (inactive)]

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 ?

Comment 20

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

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.

Comment 21

[Kilik Kuo [:kikuo] (inactive)]

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

Comment 22

[Kilik Kuo [:kikuo] (inactive)]

Attachment: [Part1]Abstract original AudioSink as base class and create DecodedAudioDataSink_r256893_v3

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

Comment 23

[Kilik Kuo [:kikuo] (inactive)]

Attachment: [Part2]Create dom/media/mediasink/moz.build and modify dom/media/moz.build according to it_256893_v3

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

Comment 24

[JW Wang [:jwwang] [:jw_wang]]

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.

Comment 25

[Kilik Kuo [:kikuo] (inactive)]

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

Comment 26

[Kilik Kuo [:kikuo] (inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4bbf8ebda9de

Comment 27

[Kilik Kuo [:kikuo] (inactive)]

Attachment: [Part1] Abstract original AudioSink as base class and create DecodedAudioDataSink_v4_r257707

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

Comment 28

[Kilik Kuo [:kikuo] (inactive)]

Attachment: [Part2]Create dom/media/mediasink/moz.build and modify dom/media/moz.build according to it_v4_257707

Carry r+ from cpearce and rebase to recent changeset.

Comment 29

[Kilik Kuo [:kikuo] (inactive)]

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

Comment 30

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 31

[Kilik Kuo [:kikuo] (inactive)]

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

Comment 32

[Kilik Kuo [:kikuo] (inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=344d0e5c6c77

Comment 33

[Kilik Kuo [:kikuo] (inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6897bb9922dc

Comment 34

[Kilik Kuo [:kikuo] (inactive)]

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

Comment 35

[Kilik Kuo [:kikuo] (inactive)]

Attachment: [Part1] Abstract original AudioSink as base class and create DecodedAudioDataSink_v5_r258072

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

Comment 36

[Kilik Kuo [:kikuo] (inactive)]

Attachment: [Part2]Create dom/media/mediasink/moz.build and modify dom/media/moz.build according to it_v5_r258072

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

Comment 37

[Kilik Kuo [:kikuo] (inactive)]

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

Comment 38

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f4091e0478f8

Comment 39

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/f4091e0478f8

Comment 40

[JW Wang [:jwwang] [:jw_wang]]

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.

Comment 41

[JW Wang [:jwwang] [:jw_wang]]

Hi Kilik,
Please check comment 40.

Comment 42

[Kilik Kuo [:kikuo] (inactive)]

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

Comment 43

[Kilik Kuo [:kikuo] (inactive)]

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

Comment 44

[JW Wang [:jwwang] [:jw_wang]]

Submit a new patch to fix the problem.

Comment 45

[Kilik Kuo [:kikuo] (inactive)]

Attachment: [Part3][3] Correct |MaybeStartPlayback()| logic while DECODER_STATE_COMPLETED_r25905

Hi JW,

please help review this correction.

Comment 46

[Kilik Kuo [:kikuo] (inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c9739ee9a48d

Comment 47

[Kilik Kuo [:kikuo] (inactive)]

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

Try run looks ok !

Comment 48

[Kilik Kuo [:kikuo] (inactive)]

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 49

[Kilik Kuo [:kikuo] (inactive)]

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

Comment 50

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a8cfe27383e3

Comment 51

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a8cfe27383e3