Closed Bug 1221991 Opened 4 years ago Closed 4 years ago

Refactor AndroidDecoderModule

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 45
Tracking Status
firefox45 --- fixed

People

(Reporter: esawin, Assigned: esawin)

Details

Attachments

(5 files, 5 obsolete files)

10.29 KB, patch
esawin
: review+
Details | Diff | Splinter Review
4.90 KB, patch
snorp
: review+
Details | Diff | Splinter Review
2.67 KB, patch
Details | Diff | Splinter Review
2.27 KB, patch
esawin
: review+
Details | Diff | Splinter Review
30.16 KB, patch
esawin
: review+
Details | Diff | Splinter Review
Some parts of AndroidDecoderModule.cpp are hard to follow, overly complex and inconsistent in style. Let's clean this module up to improve maintainability.
There is no reason I found why this function wasn't declared const in the interface. I think it should be.
Assignee: nobody → esawin
Status: NEW → ASSIGNED
Attachment #8683650 - Flags: review?(snorp)
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).
Attachment #8683653 - Flags: feedback?(snorp)
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
Attachment #8683650 - Flags: review?(snorp)
Attachment #8683650 - Flags: review?(jyavenard)
Attachment #8683650 - Flags: review+
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
Attachment #8683653 - Flags: feedback?(snorp) → feedback+
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
Attachment #8683650 - Flags: review?(jyavenard) → review-
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
(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.
Attachment #8683650 - Attachment is obsolete: true
Attachment #8685473 - Flags: review?(jyavenard)
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.
Attachment #8685473 - Flags: review?(jyavenard) → review+
Added missing FFMPEG PDM change, carrying over r+.
Attachment #8685473 - Attachment is obsolete: true
Attachment #8686028 - Flags: review+
Addressed comments.
Attachment #8683653 - Attachment is obsolete: true
Attachment #8686029 - Flags: review?(snorp)
By adding accessors for the module state, we can easily control locking and debug logging at one place.
Attachment #8686032 - Flags: feedback?(snorp)
Adds state transition logging for easier debugging.
Attachment #8686034 - Flags: feedback?(snorp)
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.
Attachment #8686037 - Flags: feedback?(snorp)
Attachment #8686029 - Flags: review?(snorp) → review+
Attachment #8686032 - Flags: feedback?(snorp) → review?(snorp)
Attachment #8686034 - Flags: feedback?(snorp) → review?(snorp)
Attachment #8686037 - Flags: feedback?(snorp)
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
Attachment #8686032 - Flags: review?(snorp) → review+
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
Attachment #8686034 - Flags: review?(snorp) → review+
Switched to NSPR-based logging. Carrying over r+.
Attachment #8686034 - Attachment is obsolete: true
Attachment #8687990 - Flags: review+
Rebased.
Attachment #8686029 - Attachment is obsolete: true
Attachment #8688547 - Flags: review+
You need to log in before you can comment on or make changes to this bug.