812032 - Refactor the media reader classes to not access MediaDecoder directly

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[(no longer active)]

For the Web Audio decoding APIs, we need to be able to use the media reader classes, but we don't want to use MediaDecoder itself, since the state management logic in there is not useful for decoding a whole buffer.  So the idea here is to refactor the stuff that the reader classes use into an abstract base class which can be implemented separately by the web audio decoding code.

Comment 1

[(no longer active)]

Attachment: Patch (v1)

Comment 2

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

Comment on attachment 681798 [details] [diff] [review]
Patch (v1)

Review of attachment 681798 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/media/AbstractDecoder.h
@@ +23,5 @@
> +/**
> + * The AbstractDecoder class describes the public interface for a media decoder
> + * and is used by the MediaReader classes.
> + */
> +class AbstractDecoder : public nsISupports

AbstractMediaDecoder. It's not an image or ZIP decoder. ;)

@@ +49,5 @@
> +  // Increments the parsed and decoded frame counters by the passed in counts.
> +  // Can be called on any thread.
> +  virtual void NotifyDecodedFrames(uint32_t aParsed, uint32_t aDecoded) = 0;
> +
> +  virtual int64_t GetEndMediaTime() const = 0;

Add comment:

Returns the end time of the last sample in the media. Note that a media can have a non-zero start time, so the end time may not necessarily be the same as the duration (i.e. duration is (end_time - start_time)).

@@ +54,5 @@
> +
> +  // Set the duration of the media resource in units of seconds.
> +  // This is called via a channel listener if it can pick up the duration
> +  // from a content header. Must be called from the main thread only.
> +  virtual void SetDuration(double aDuration) = 0;

So this is the main-thread version of SetDuration(double durationInSecs) which is called if we detect a Content-Duration HTTP header, which (before your patch) acquires the decoder monitor and calls the state machine's SetDuration(int64_t durationInUsecs). So the places where we're currently calling MediaDecoderStateMachine::SetDuration(int64) which your patch changes to call AbstractMediaDecoder::SetDuration(double) are now passing the duration in the wrong units to a function which asserts that it's on the main thread (an assertion which should be failing because we call SetDuration(int64) on the decode thread).

So I think you should change AbstractMediaDecoder::SetDuration() to take a int64_t usecs parameter, and pass that through to the state machine's implementation, and change the caller of the SetDuration(double) (MediaResource and MediaElement I think) to pass in usecs instead of seconds.

Can you also change AbstractMediaDecoder::GetDuration() to return a value in usecs and have the caller (media element) handle the conversion to seconds instead? Then we're consistent, and only those who want the duration in seconds needs to worry about converting it.

@@ +79,5 @@
> +  // Called when the metadata from the media file has been read by the reader.
> +  // Call on the decode thread only.
> +  virtual void OnReadMetadataCompleted() = 0;
> +
> +  virtual void NextFrameAvailable() = 0;

So the only backend which uses this function is the GStreamerReader and I'm sure GStreamerReader *shouldn't* be using it; all the HTML event/state logic should be in MediaDecoderStateMachine, and so only MediaDecoderStateMachine should call this. So remove the calls to NextFrameAvailable from GStreamerReader, and you can remove NextFrameAvailable from this interface. I ran tests with the calls of NextFrameAvailable removed and we still pass tests with GStreamer enabled (and we still hang on shutdown FWIW).

::: content/media/MediaDecoder.h
@@ +225,5 @@
>    return NS_GetCurrentThread() == aThread;
>  }
>  
> +class MediaDecoder : public nsIObserver,
> +                     public mozilla::AbstractDecoder

You shouldn't need the "mozilla::" on AbstractDecoder?

Can you remove the extraneous "virtual" keywords here, and add MOZ_OVERRIDE where appropriate?

@@ +548,4 @@
>    // state.
>    virtual ReentrantMonitor& GetReentrantMonitor();
>  
> +  // Returns true if the decoder is shut down

Nit: full stop at end of sentences please.

::: content/media/gstreamer/GStreamerReader.cpp
@@ +393,4 @@
>  
>    if (mAudioQueue.GetSize() < 2) {
>      nsCOMPtr<nsIRunnable> event =
> +      NS_NewRunnableMethod(mDecoder, &AbstractDecoder::NextFrameAvailable);

This whole if block should be removed.

@@ +491,2 @@
>  
>    if (mVideoQueue.GetSize() < 2) {

This whole if block should be removed.

Comment 3

[(no longer active)]

Attachment: Patch (v2)

Comment 4

[(no longer active)]

https://tbpl.mozilla.org/?tree=Try&rev=2d3ad7bddb47

Comment 5

[(no longer active)]

Attachment: Additional changes needed to build for b2g

I'll land this as part of the same patch, obviously.

Comment 6

[Mozilla RelEng Bot]

Try run for 2d3ad7bddb47 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=2d3ad7bddb47
Results (out of 98 total builds):
    success: 87
    warnings: 9
    failure: 1
    other: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-2d3ad7bddb47

Comment 7

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

Comment on attachment 682173 [details] [diff] [review]
Patch (v2)

Review of attachment 682173 [details] [diff] [review]:
-----------------------------------------------------------------

r=cpearce with the changes specified below.

::: content/media/AbstractMediaDecoder.h
@@ +55,5 @@
> +  // the same as the duration (i.e. duration is (end_time - start_time)).
> +  virtual int64_t GetEndMediaTime() const = 0;
> +
> +  // Return the duration of the video in seconds.
> +  virtual int64_t GetStateMachineDuration() = 0;

Rename this to GetMediaDuration().

This returns the duration in microseconds, not seconds; the comment is wrong and should be changed.

@@ +57,5 @@
> +
> +  // Return the duration of the video in seconds.
> +  virtual int64_t GetStateMachineDuration() = 0;
> +
> +  virtual void SetStateMachineDuration(int64_t aDuration) = 0;

SetMediaDuration()

Add a comment saying that aDuration is in microseconds.

@@ +63,5 @@
> +  virtual VideoFrameContainer* GetVideoFrameContainer() = 0;
> +  virtual mozilla::layers::ImageContainer* GetImageContainer() = 0;
> +
> +  // Return true if seeking is supported.
> +  virtual bool IsStateMachineSeekable() = 0;

IsMediaSeekable()

@@ +65,5 @@
> +
> +  // Return true if seeking is supported.
> +  virtual bool IsStateMachineSeekable() = 0;
> +
> +  virtual void SetStateMachineEndTime(double aTime) = 0;

There's two forms of SetEndTime(), one which takes a double, the other takes an in64_t.

MediaDecoder::SetEndTime(double) sets the media fragment end time, it calls MediaDecoderStateMachine::SetFragmentEndTime(). This is the time at which we stop playback if playing a media fragment. Please rename MediaDecoder::SetEndTime(double) to MediaDecoder::SetFragmentEndTime(double).

MediaDecoderStateMachine::SetEndTime(int64_t) sets the last known timestamp in the media. This is used to calculate the duration, etc. Please rename 
MediaDecoderStateMachine::SetEndTime(int64_t) and AbstractMediaDecoder::SetStateMachineEndTime(int64_t) to SetMediaEndTime(int64_t). The parameter should be int64_t parameter in microseconds units, not a double.

Add a comment: Sets the timestamp at which the last sample in the media ends, in microseconds.

::: content/media/MediaDecoder.h
@@ +510,3 @@
>    // Set the end time of the media resource. When playback reaches
>    // this point the media pauses. aTime is in seconds.
>    virtual void SetEndTime(double aTime);

SetFragmentEndTime(double)

@@ +510,4 @@
>    // Set the end time of the media resource. When playback reaches
>    // this point the media pauses. aTime is in seconds.
>    virtual void SetEndTime(double aTime);
> +  void SetStateMachineEndTime(double aTime) MOZ_FINAL MOZ_OVERRIDE;

SetMediaEndTime(int64 aTimeUs)

Comment 8

[(no longer active)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/41bc47dfa5b2

Comment 9

[(no longer active)]

Chris, I sort of screwed up here!  I thought that you had reviewed both of the patches, but the review on the second patch is still pending.  Do you want me to back it out or do you wanna post-review it?  Sorry for the mistake, sleep deprivation finally got to me.  :/

Comment 10

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

Comment on attachment 682216 [details] [diff] [review]
Additional changes needed to build for b2g

Review of attachment 682216 [details] [diff] [review]:
-----------------------------------------------------------------

Don't worry about it. :)

Comment 11

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/41bc47dfa5b2

Comment 12

[Mozilla RelEng Bot]

Try run for 2d3ad7bddb47 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=2d3ad7bddb47
Results (out of 99 total builds):
    success: 87
    warnings: 9
    failure: 2
    other: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-2d3ad7bddb47