Closed Bug 1349883 Opened 7 years ago Closed 7 years ago

Make RemoteDataDecoder resolves promise properly.

Categories

(Firefox for Android Graveyard :: Audio/Video, enhancement, P1)

Product:
Firefox for Android Graveyard
Component:
Audio/Video
Type:
enhancement

Tracking

(firefox54 fixed, firefox55 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 55
Tracking Flags:
Tracking Status
firefox54 --- fixed
firefox55 --- fixed

People

(Reporter: jhlin, Assigned: jhlin)

References

Details

Attachments

(4 files, 1 obsolete file)

Bug 1349883 - part 1: combine output buffer index and sample records.
59 bytes, text/x-review-board-request
Details
Bug 1349883 - part 2: reveal more input buffer status to callbacks.
59 bytes, text/x-review-board-request
Details
Bug 1349883 - part 3: resolve decode promise according to buffer status.
59 bytes, text/x-review-board-request
jya
: review+
Details
Bug 1349883 - part 4: strengthen precondition of ReturnDecodedData().
59 bytes, text/x-review-board-request
jya
: review+
Details 
      No description provided.
Comment on attachment 8850455 [details]
Bug 1349883 - Let RemoteDataDecoder::Decode() resolve promise with exising decoded data.

https://reviewboard.mozilla.org/r/123050/#review125350

::: dom/media/platforms/android/RemoteDataDecoder.cpp:605
(Diff revision 1)
> -           : DecodePromise::CreateAndReject(
>                 MediaResult(NS_ERROR_OUT_OF_MEMORY, __func__), __func__);
> +    }
>  
> +    RefPtr<DecodePromise> p = mDecodePromise.Ensure(__func__);
> +    if (!mDecodedData.IsEmpty()) {

I don't believe this can ever be true.
As if you had a frame, it would already have been returned via OnInputExhausted or HandleOutput().

Now if we use the assumption that you did have a frame ready to be returned.
All this does is delay the soon to be decoded frame to be returned after the next call to Decode.

Rather than returning 2 samples at once, you return only one, and the next one can no longer be returned. You've added latency.

The total times after this can only be the same at best.

Additionally, having multiple code path to return frames only muddy the context. It should all be done in the same spot, all at once.
Attachment #8850455 - Flags: review?(jyavenard) → review-
Attachment #8850455 - Attachment is obsolete: true
Got suggestions from jya on IRC for this bug.
Assignee: nobody → jolin
Status: NEW → ASSIGNED
Summary: Let RemoteDataDecoder::Decode() resolve promise immediately with existing decoded data → Let RemoteDataDecoder resolves promise properly.
Blocks: 1348864
Current implementation doesn't work very well because OOP decoder sends outputs ASAP so there are chances that multiple outputs arrive when a promise is pending. In this case the 1st one will be returned, but others have to wait until next promise is created *and* decoder requests more input or sends another output after that.

Expected flow:
1. RemoteDataDecoder::Decode() => promise pending

2. JavaCallbacksSupport::HandleInputExhausted() or JavaCallbacksSupport::HandleOutput() => promise resolved

3. back to 1.


Problematic flow:
1. RemoteDataDecoder::Decode() => promise pending

2. JavaCallbacksSupport::HandleInputExhausted() or JavaCallbacksSupport::HandleOutput() => promise resolved

3. HandleOutput() => no promise to resolve in ReturnDecodedData()

4. RemoteDataDecoder::Decode() => promise pending until HandleInputExhausted() or HandleOutput() arrives
   - no HandleInputExhausted() because there are already some (2) inputs waiting in queue
   - no HandleOutput() because previous output pending in mDecodedData
Summary: Let RemoteDataDecoder resolves promise properly. → Make RemoteDataDecoder resolves promise properly.
Why would you have no HandleOutput when you have something in mDecodedData?
Priority: -- → P1
(In reply to Jean-Yves Avenard [:jya] from comment #5)
> Why would you have no HandleOutput when you have something in mDecodedData?

So far two different scenario are observed. The 1st one: when video resolutions changed, the OMX decoder will enter internal flushing state and will not proceed until all output buffers with old size are returned. The 2nd is still under investigation.
The 2nd scenario happens when video track was resumed from suspension (bug 1348864). When that happens we'll seek video track to current audio playback time to archive A/V sync.

In current implementation, MDSM stores latest returned frame in mFirstVideoFrameAfterSeek [1] before seek complete. That, plus the one pending in mDecodedData, consumes 2 out of 5 output buffers that OMX decoder uses. It seems Android decoding machinery don't like consumer holding multiple buffers.

[1] http://searchfox.org/mozilla-central/source/dom/media/MediaDecoderStateMachine.cpp#1391
Blocks: 1348085
Comment on attachment 8855672 [details]
Bug 1349883 - part 3: resolve decode promise according to buffer status.

https://reviewboard.mozilla.org/r/127534/#review130218

::: dom/media/platforms/android/RemoteDataDecoder.cpp:463
(Diff revision 1)
>  RefPtr<MediaDataDecoder::DecodePromise>
>  RemoteDataDecoder::Drain()
>  {
>    RefPtr<RemoteDataDecoder> self = this;
>    return InvokeAsync(mTaskQueue, __func__, [self, this]() {
> +    if (mShutdown) {

that seems out of scope, and likely would be better done in a separate patch, with its own description

::: dom/media/platforms/android/RemoteDataDecoder.cpp:568
(Diff revision 1)
>      return;
>    }
> -  mDecodedData.AppendElement(aSample);
> +
> +  if (!aProcessed) {
> +    mNumPendingInputs++;
> +  } else if (mNumPendingInputs > 0) {

shouldn't we assert that mNumPendingInput isn't 0 here?

::: dom/media/platforms/android/RemoteDataDecoder.cpp:573
(Diff revision 1)
> +  } else if (mNumPendingInputs > 0) {
> +    mNumPendingInputs--;
> +  }
> +
> +  if (mNumPendingInputs == 0 // Input has been processed, request the next one.
> +      || !mDecodedData.IsEmpty()) { // Previous output arrived before Decode().

actually || needs to be at the end of the previous line

::: mobile/android/base/java/org/mozilla/gecko/media/CodecProxy.java:73
(Diff revision 1)
> -                mCallbacks.onInputExhausted();
> +                mCallbacks.onInputStatus(timestamp, true /* processed */);
>              }
>          }
>  
>          @Override
>          public synchronized void onInputPended(long timestamp) throws RemoteException {

should have been onInputPending.

Pended isn't a word :)
Attachment #8855672 - Flags: review?(jyavenard) → review+
Comment on attachment 8855672 [details]
Bug 1349883 - part 3: resolve decode promise according to buffer status.

https://reviewboard.mozilla.org/r/127534/#review130218

> that seems out of scope, and likely would be better done in a separate patch, with its own description

I did consider making it a seperate patch. But since this is added in order to convert the checking to assertion in `ReturnDecodedData()`, it seems out of context on its own too. :)

> shouldn't we assert that mNumPendingInput isn't 0 here?

`mNumPendingInput` could be 0 for some inputs (before 1st pending buffer or right after all pending buffers cleared).
Comment on attachment 8855672 [details]
Bug 1349883 - part 3: resolve decode promise according to buffer status.

https://reviewboard.mozilla.org/r/127534/#review130218

> I did consider making it a seperate patch. But since this is added in order to convert the checking to assertion in `ReturnDecodedData()`, it seems out of context on its own too. :)

I'll move both this and the change in `ReturnDecodedData()` to another patch.
Comment on attachment 8855709 [details]
Bug 1349883 - part 4: strengthen precondition of ReturnDecodedData().

https://reviewboard.mozilla.org/r/127592/#review130286

::: dom/media/platforms/android/RemoteDataDecoder.cpp:465
(Diff revision 1)
>  {
>    RefPtr<RemoteDataDecoder> self = this;
>    return InvokeAsync(mTaskQueue, __func__, [self, this]() {
> +    if (mShutdown) {
> +      return DecodePromise::CreateAndReject(
> +               MediaResult(NS_ERROR_DOM_MEDIA_CANCELED, __func__), __func__);

If you're there, the DrainPromise has been disconnected, what you return is of not interest as it can't be used anywhere else.

so there's no need to return MediaResult(nsresult, __func__)

nsresult is directly convertible into a MediaResult

so return DecodePromise::CreateAndReject(NS_ERROR_DOM_MEDIA_CANCELED, __func__);
Attachment #8855709 - Flags: review?(jyavenard) → review+
Comment on attachment 8855709 [details]
Bug 1349883 - part 4: strengthen precondition of ReturnDecodedData().

https://reviewboard.mozilla.org/r/127592/#review130818

::: commit-message-e7529:1
(Diff revision 2)
> +Bug 1349883 - part 4: strengthen precondition of ReturnDecodedData(). r?jya

strengthen
Comment on attachment 8855709 [details]
Bug 1349883 - part 4: strengthen precondition of ReturnDecodedData().

https://reviewboard.mozilla.org/r/127592/#review130836

::: dom/media/platforms/android/RemoteDataDecoder.cpp:599
(Diff revision 2)
> -    mTaskQueue->Dispatch(
> -      NewRunnableMethod(this, &RemoteDataDecoder::ReturnDecodedData));
> -    return;
> -  }
>    AssertOnTaskQueue();
> -  if (mShutdown) {
> +  MOZ_ASSERT(!mShutdown);

I don't think we should assert here.

While mCancel in the callback class is an Atomic; it could be that the Callback is currently running when ProcessShutdown is being called.

so it is possible that mShutdown is true when ReturnDecodedData is being called.
Comment on attachment 8855709 [details]
Bug 1349883 - part 4: strengthen precondition of ReturnDecodedData().

https://reviewboard.mozilla.org/r/127592/#review130836

> I don't think we should assert here.
> 
> While mCancel in the callback class is an Atomic; it could be that the Callback is currently running when ProcessShutdown is being called.
> 
> so it is possible that mShutdown is true when ReturnDecodedData is being called.

Yes callback could be running when shutting down, but now all call sites of `ReturnDecodedData` check `mShutdown` before calling it, therefore the assertion.
Comment on attachment 8855709 [details]
Bug 1349883 - part 4: strengthen precondition of ReturnDecodedData().

https://reviewboard.mozilla.org/r/127592/#review130836

> Yes callback could be running when shutting down, but now all call sites of `ReturnDecodedData` check `mShutdown` before calling it, therefore the assertion.

Then, if ReturnDecodedData is only ever called on the taskqueue, we should assert that it's on the taskqueue and remove the test that may dispatch a new task.

As it is, ReturnDecodedData looks like it could be called on different threads, in which case, you can't guarantee that mShutdown hasn't changed since it was last checked
Comment on attachment 8855670 [details]
Bug 1349883 - part 1: combine output buffer index and sample records.

https://reviewboard.mozilla.org/r/127530/#review130888

I really shouldn't be reviewing these anymore. I recommend esawin or jya (or both).
Comment on attachment 8855670 [details]
Bug 1349883 - part 1: combine output buffer index and sample records.

https://reviewboard.mozilla.org/r/127530/#review130890
Attachment #8855670 - Flags: review?(snorp) → review+
Attachment #8855670 - Flags: review?(jyavenard)
Attachment #8855670 - Flags: review?(esawin)
Attachment #8855671 - Flags: review?(snorp)
Attachment #8855671 - Flags: review?(jyavenard)
Attachment #8855671 - Flags: review?(esawin)
Comment on attachment 8855670 [details]
Bug 1349883 - part 1: combine output buffer index and sample records.

https://reviewboard.mozilla.org/r/127530/#review131252

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/Codec.java:282
(Diff revision 2)
>          }
>  
>          private synchronized void reset() {
> -            for (int i : mSentIndices) {
> -                mCodec.releaseOutputBuffer(i, false);
> -            }
> +            for (Output o : mSentOutputs) {
> +                mCodec.releaseOutputBuffer(o.index, false);
> +                mSamplePool.recycleOutput(o.sample);

I don't know much about java, but can't we have a Output destructor that would do this operation automatically upon calling mSentOutputs.clear() ?
Attachment #8855670 - Flags: review?(jyavenard) → review+
Comment on attachment 8855670 [details]
Bug 1349883 - part 1: combine output buffer index and sample records.

https://reviewboard.mozilla.org/r/127530/#review131252

> I don't know much about java, but can't we have a Output destructor that would do this operation automatically upon calling mSentOutputs.clear() ?

Java uses garbage collection so the objects hold by collection won't be destruct/free by clear(). Instead, they lives until next GC is performed. Explictly releasing the buffers frees native resources (memory, fd, ...) as soon as possible.
Comment on attachment 8855671 [details]
Bug 1349883 - part 2: reveal more input buffer status to callbacks.

https://reviewboard.mozilla.org/r/127532/#review131254

LGTM...

though I'm not sure I understand 100% what you're attempting to achieve here.
You're reporting the pts back to the callback, but the pts doesn't give much information in regards to where in the *decoding* queue that sample is, which would allow the RemoteDataDecoder to make a determination on when to resolve the promise.

InputExhausted() is designed to mean: "I, the decoder, can't do anything anymore unless a new input is provided. I won't output a new sample until then"

It doesn't appear that you're exposing that information back to the RemoteDataDecoder, so until then, it all seems like a workaround to me that gets us close to the intended behaviour, but one that doesn't resolve the problem 100%.

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/Codec.java:173
(Diff revision 3)
> +                        i.reported = true;
> +                        mCallbacks.onInputPending(i.sample.info.presentationTimeUs);
> +                    }
> +                }
> +            } catch (RemoteException e) {
> +                    e.printStackTrace();

indent
Attachment #8855671 - Flags: review?(jyavenard) → review+
Comment on attachment 8855670 [details]
Bug 1349883 - part 1: combine output buffer index and sample records.

https://reviewboard.mozilla.org/r/127530/#review131442

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/Codec.java:192
(Diff revision 2)
>              reset();
>          }
>      }
>  
> +    private static class Output {
> +        public Sample sample;

Final.

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/Codec.java:193
(Diff revision 2)
>          }
>      }
>  
> +    private static class Output {
> +        public Sample sample;
> +        public int index;

Final.

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/Codec.java:195
(Diff revision 2)
>  
> +    private static class Output {
> +        public Sample sample;
> +        public int index;
> +
> +        public Output(Sample sample, int index) {

Final.

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/Codec.java:259
(Diff revision 2)
>  
>              return sample;
>          }
>  
>          private synchronized void onRelease(Sample sample, boolean render) {
> -            Integer i = mSentIndices.poll();
> +            Output output = mSentOutputs.poll();

Final.

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/Codec.java:280
(Diff revision 2)
>                  re.printStackTrace();
>              }
>          }
>  
>          private synchronized void reset() {
> -            for (int i : mSentIndices) {
> +            for (Output o : mSentOutputs) {

Final.
Attachment #8855670 - Flags: review?(esawin) → review+
Comment on attachment 8855671 [details]
Bug 1349883 - part 2: reveal more input buffer status to callbacks.

https://reviewboard.mozilla.org/r/127532/#review131444

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/Codec.java:54
(Diff revision 3)
>              mOutputProcessor.onFormatChanged(format);
>          }
>      }
>  
> +    private static class Input {
> +        public Sample sample;

Final.

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/Codec.java:55
(Diff revision 3)
>          }
>      }
>  
> +    private static class Input {
> +        public Sample sample;
> +        public boolean reported;

Final.

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/Codec.java:57
(Diff revision 3)
>  
> +    private static class Input {
> +        public Sample sample;
> +        public boolean reported;
> +
> +        public Input(Sample sample) {

Final.

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/Codec.java:134
(Diff revision 3)
>  
>          private void feedSampleToBuffer() {
>              while (!mAvailableInputBuffers.isEmpty() && !mInputSamples.isEmpty()) {
>                  int index = mAvailableInputBuffers.poll();
>                  int len = 0;
> -                Sample sample = mInputSamples.poll();
> +                Sample sample = mInputSamples.poll().sample;

Final.
Attachment #8855671 - Flags: review?(esawin) → review+
Comment on attachment 8855671 [details]
Bug 1349883 - part 2: reveal more input buffer status to callbacks.

https://reviewboard.mozilla.org/r/127532/#review131444

> Final.

`reported` will be marked after construction.
Pushed by jolin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8e0bcd410a17
part 1: combine output buffer index and sample records. r=esawin,jya,snorp
https://hg.mozilla.org/integration/autoland/rev/40fbb04e0e65
part 2: reveal more input buffer status to callbacks. r=esawin,jya
https://hg.mozilla.org/integration/autoland/rev/62a60bce1f9c
part 3: resolve decode promise according to buffer status. r=jya
https://hg.mozilla.org/integration/autoland/rev/aca4c82e3d70
part 4: strengthen precondition of ReturnDecodedData(). r=jya
Blocks: 1356539
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: