1221991 - Refactor AndroidDecoderModule

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Audio/Video, defect)

Description

[Eugen Sawin [:esawin]]

Some parts of AndroidDecoderModule.cpp are hard to follow, overly complex and inconsistent in style. Let's clean this module up to improve maintainability.

Comment 1

[Eugen Sawin [:esawin]]

Attachment: 0001-Bug-1221991-1.1-Make-SupportsMimeType-a-const-functi.patch

There is no reason I found why this function wasn't declared const in the interface. I think it should be.

Comment 2

[Eugen Sawin [:esawin]]

Attachment: 0002-Bug-1221991-2.1-Split-DecoderLoop-into-functions-sim.patch

DecoderLoop is 177 lines long, difficult to read and maintain. This patch splits some of the "verbs" out of the loop into separate functions and introduces a single state variable (mState) to keep track of the decoding status.

We also don't lock the monitor for mDurations access, and I think we should.

There are some things I'm not happy about yet:
1. Locking mechanics - this is still somewhat inconsistent as some function lock mMonitor and others require it to be locked before calling.
2. Error handling - the macro has to go, it's also not thread-safe.
3. Still too complex decoder state - I would like to reduce the number of states we need without sacrificing expressiveness.
4. Inconsistent argument passing (QueueSample takes a reference).

Comment 3

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8683650 [details] [diff] [review]
0001-Bug-1221991-1.1-Make-SupportsMimeType-a-const-functi.patch

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

Looks ok to me, but we need a media person to review

Comment 4

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8683653 [details] [diff] [review]
0002-Bug-1221991-2.1-Split-DecoderLoop-into-functions-sim.patch

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

This is definitely an improvement. I think I'd like to see more asserts around state transitions.

::: dom/media/platforms/android/AndroidDecoderModule.cpp
@@ +515,3 @@
>  
> +  if (inputIndex < 0) {
> +    // There is no valid input buffer available, drop sample but keep decoding.

We can't drop it. We need to put it back into mQueue or save it somewhere so it can be enqueued when a buffer is available in the decoder.

EDIT: I see now this probably doesn't get dropped. Comment is just incorrect.

@@ +532,5 @@
>  
> +  {
> +    // We're feeding this to the decoder, so remove it from the queue.
> +    MonitorAutoLock lock(mMonitor);
> +    mQueue.pop();

It seems wrong for QueueSample() to do anything with mQueue. I think that part should be managed outside of this function.

@@ +576,5 @@
> +MediaCodecDataDecoder::HandleEOS(int32_t aOutputStatus)
> +{
> +  MonitorAutoLock lock(mMonitor);
> +
> +  if (mState == kDraining || mState == kWaitingEOS) {

Is it possible to get here with mState = kDraining? It seems like we shouldn't...

::: dom/media/platforms/android/AndroidDecoderModule.h
@@ +61,5 @@
>  protected:
> +  enum State {
> +    kDecoding,
> +    kFlushing,
> +    kDrainingRequested,

kDrainRequested

Comment 5

[Jean-Yves Avenard [:jya]]

Comment on attachment 8683650 [details] [diff] [review]
0001-Bug-1221991-1.1-Make-SupportsMimeType-a-const-functi.patch

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

::: dom/media/platforms/PDMFactory.h
@@ +35,4 @@
>                  layers::LayersBackend aLayersBackend = layers::LayersBackend::LAYERS_NONE,
>                  layers::ImageContainer* aImageContainer = nullptr);
>  
> +  bool SupportsMimeType(const nsACString& aMimeType) const;

making it const is going to make the WMF one difficult to implement as the WMF creates decoder and isn't "const".

You've also missed the ffmpeg, apple and wmf PDM

Comment 6

[Jean-Yves Avenard [:jya]]

Comment on attachment 8683653 [details] [diff] [review]
0002-Bug-1221991-2.1-Split-DecoderLoop-into-functions-sim.patch

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

::: dom/media/platforms/PlatformDecoderModule.h
@@ +53,4 @@
>    virtual nsresult Startup() { return NS_OK; };
>  
>    // Indicates if the PlatformDecoderModule supports decoding of aMimeType.
> +  virtual bool SupportsMimeType(const nsACString& aMimeType) const = 0;

this should have been put in the other patch, but as commented, that's too restrictive on how SupportsMimeType can be implemented across all platforms

Comment 7

[Eugen Sawin [:esawin]]

Attachment: 0001-Bug-1221991-1.2-Make-SupportsMimeType-a-const-functi.patch

(In reply to Jean-Yves Avenard [:jya] from comment #5)
> making it const is going to make the WMF one difficult to implement as the
> WMF creates decoder and isn't "const".

CanCreateWMFDecoder() is static, so the WMF implementation is const.

> You've also missed the ffmpeg, apple and wmf PDM

Thanks, not sure how I've missed those.

Enforcing const-correctness would help reduce unwanted side-effects and increase expressiveness, let's do it step by step.

If an implementation introduces non-const behavior (and therefore potential side effects), it should indicate that by using mutable state.

Comment 8

[Jean-Yves Avenard [:jya]]

Comment on attachment 8685473 [details] [diff] [review]
0001-Bug-1221991-1.2-Make-SupportsMimeType-a-const-functi.patch

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

you're still missing ffmpeg PDM.

Comment 9

[Eugen Sawin [:esawin]]

Attachment: 0001-Bug-1221991-1.3-Make-SupportsMimeType-a-const-functi.patch

Added missing FFMPEG PDM change, carrying over r+.

Comment 10

[Eugen Sawin [:esawin]]

Attachment: 0002-Bug-1221991-2.2-Split-DecoderLoop-into-functions-sim.patch

Addressed comments.

Comment 11

[Eugen Sawin [:esawin]]

Attachment: 0003-Bug-1221991-3.1-Manage-module-state-via-accessor-fun.patch

By adding accessors for the module state, we can easily control locking and debug logging at one place.

Comment 12

[Eugen Sawin [:esawin]]

Attachment: 0004-Bug-1221991-4.1-Add-AndroidDecoderModule-logging.-r-.patch

Adds state transition logging for easier debugging.

Comment 13

[Eugen Sawin [:esawin]]

Attachment: 0005-Bug-1221991-5.1-Enable-lock-free-module-state-handli.patch

Use atomic for lock-free state transitions.

This is not completely lock-free yet because of missing thread:: support for the yield/sleep flushing cycles.
I've had issues getting this to work with sched_yield, but that's something we could look into.

Comment 14

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8686032 [details] [diff] [review]
0003-Bug-1221991-3.1-Manage-module-state-via-accessor-fun.patch

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

::: dom/media/platforms/android/AndroidDecoderModule.cpp
@@ +704,5 @@
> +
> +void
> +MediaCodecDataDecoder::State(ModuleState aState)
> +{
> +  if (aState == kDrainDecoder) {

It seems like there are there more assertions we could make, but this seems like a good start

Comment 15

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8686034 [details] [diff] [review]
0004-Bug-1221991-4.1-Add-AndroidDecoderModule-logging.-r-.patch

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

We probably want this off by default, but I love the idea

Comment 16

[Eugen Sawin [:esawin]]

Attachment: 0004-Bug-1221991-4.2-Add-AndroidDecoderModule-logging.-r-.patch

Switched to NSPR-based logging. Carrying over r+.

Comment 17

[Eugen Sawin [:esawin]]

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

Comment 18

[Eugen Sawin [:esawin]]

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

Comment 19

[Eugen Sawin [:esawin]]

Attachment: 0002-Bug-1221991-2.3-Split-DecoderLoop-into-functions-sim.patch

Rebased.

Comment 20

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0899a0ebe697
https://hg.mozilla.org/integration/mozilla-inbound/rev/980de2427838
https://hg.mozilla.org/integration/mozilla-inbound/rev/b680d92e4afc
https://hg.mozilla.org/integration/mozilla-inbound/rev/439f35445959

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0899a0ebe697
https://hg.mozilla.org/mozilla-central/rev/980de2427838
https://hg.mozilla.org/mozilla-central/rev/b680d92e4afc
https://hg.mozilla.org/mozilla-central/rev/439f35445959