1178682 - MediaCodec: GonkAudioDecoderManager: invalid decoded data with EOS flag.

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Benjamin Chen [:bechen]]

See bug 1140995 comment 3.

Comment 1

[Benjamin Chen [:bechen]]

Attachment: bug-1178682.v01.patch

Comment 2

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8629859 [details] [diff] [review]
bug-1178682.v01.patch

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

::: dom/media/platforms/gonk/GonkAudioDecoderManager.cpp
@@ +166,5 @@
>    }
>  
> +  if (mLastDecodedTime > timeUs) {
> +    ReleaseAudioBuffer();
> +    GADM_LOG("Output decoded sample time is revert. time=%lld", timeUs);

Can you explain why this happen?

Comment 3

[Benjamin Chen [:bechen]]

(In reply to Sotaro Ikeda [:sotaro] from comment #2)
> Comment on attachment 8629859 [details] [diff] [review]
> bug-1178682.v01.patch
> 
> Review of attachment 8629859 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/media/platforms/gonk/GonkAudioDecoderManager.cpp
> @@ +166,5 @@
> >    }
> >  
> > +  if (mLastDecodedTime > timeUs) {
> > +    ReleaseAudioBuffer();
> > +    GADM_LOG("Output decoded sample time is revert. time=%lld", timeUs);
> 
> Can you explain why this happen?

I don't know the exact reason why MediaServer/MediaCodec returns such kind of decoded sample.
On emulator-kk build or flame-kk build, I found the last decoded sample (with EOS flag) is always not a valid sample. It contains the revert timestamp that make something wrong if we send the sample to MDSM. See bug 1140995. (It looks like the MediaServer/MediaCodec just take an old MediaBuffer and raise the EOS flag)

This path will reject the sample with revert timestamp, the same fix had already exist in GonkVideoDecoderManager.cpp. 
(Note that the solution can't handle nagtive timestamp and chained file, bug 1140995 comment 7, bug 1140995 comment 10)

Comment 4

[Sotaro Ikeda [:sotaro]]

It might be better to analyze why it happen and fix the root cause. One of my concern is that this change does not fix the root cause of the problem. And the change might mask a drain and flush related bug in future. Is it possible to debug the root cause of the time stamp revert?

Comment 5

[Sotaro Ikeda [:sotaro]]

(In reply to Benjamin Chen [:bechen] from comment #3)
> On emulator-kk build or flame-kk build, I found the last decoded sample
> (with EOS flag) is always not a valid sample. It contains the revert
> timestamp that make something wrong if we send the sample to MDSM. See bug
> 1140995. (It looks like the MediaServer/MediaCodec just take an old
> MediaBuffer and raise the EOS flag)

Even when we do some kinds of workaround to the problem  it seems important to understand the root cause. If we understand the root cause, we could know easily what is best workaround for it. And if the timestamp problem happens only to last decoded sample, the workaround should limit to a sample with EOS.

Comment 6

[Sotaro Ikeda [:sotaro]]

And the problematic last decoded sample has valid data?

Comment 7

[Benjamin Chen [:bechen]]

LOG in ACodec.cpp

ACodec  : [OMX.google.aac.decoder] onOMXFillBufferDone 0xb0cd52e0 time 5482666 us, flags = 0x00000000 rangeLength 2048
ACodec  : [OMX.google.aac.decoder] onOMXFillBufferDone 0xb0cd5330 time 5504000 us, flags = 0x00000000 rangeLength 2048
ACodec  : [OMX.google.aac.decoder] onOMXFillBufferDone 0xb0cd5380 time 5525333 us, flags = 0x00000000 rangeLength 2048
ACodec  : [OMX.google.aac.decoder] onOMXFillBufferDone 0xb0cd5240 time 5546666 us, flags = 0x00000000 rangeLength 2048
ACodec  : [OMX.google.aac.decoder] onOMXFillBufferDone 0xb0cd52e0 time 5568000 us, flags = 0x00000000 rangeLength 2048
ACodec  : [OMX.google.aac.decoder] onOMXFillBufferDone 0xb0cd5330 time 5504000 us, flags = 0x00000001 rangeLength 2048
07-08 10:42:35.470   700   989 E ACodec  : [OMX.google.aac.decoder] saw output EOS

Comment 8

[Sotaro Ikeda [:sotaro]]

What is the root cause of this? Investigation seems not very difficult in this case.

Comment 9

[Benjamin Chen [:bechen]]

(In reply to Sotaro Ikeda [:sotaro] from comment #8)
> What is the root cause of this? Investigation seems not very difficult in
> this case.

I'm still debugging the ACodec.cpp , but the log in comment 7 shows that the last sample's rangeLenth is 2048, so I should dig into SoftAAC2.cpp.

Comment 10

[Benjamin Chen [:bechen]]

Attachment: 0001-Add-timestamp-for-the-last-decoded-sample.patch

Add timestamp on the samples after flush.

Comment 11

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8631452 [details] [diff] [review]
0001-Add-timestamp-for-the-last-decoded-sample.patch

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

Is this a patch for emulator-kk? It uses SoftAAC2.cpp of kk aosp. The code seems to have a defect related to EOS handling. And the problem seems to be fixed by kk caf code. Can's we use caf's fix, can we? It seems stable fix .


SoftAAC2.cpp of L(5.1/5.0) aosp code seems to be changed significantly since kk aosp and the EOS handling problem seems to be fixed.

kk aosp code.
   https://github.com/mozilla-b2g/platform_frameworks_av/blob/b2g-4.4.2_r1/media/libstagefright/codecs/aacdec/SoftAAC2.cpp
kk caf code
   https://www.codeaurora.org/cgit/external/gigabyte/platform/frameworks/av/tree/media/libstagefright/codecs/aacdec/SoftAAC2.cpp?h=caf%2Fb2g_kk_3.5
L(5.1) aosp code
   http://androidxref.com/5.1.1_r6/xref/frameworks/av/media/libstagefright/codecs/aacdec/SoftAAC2.cpp

::: media/libstagefright/codecs/aacdec/SoftAAC2.cpp
@@ +396,1 @@
>              } else {

else case seems not handled in this patch. kk caf's code handle this case.

Comment 12

[Benjamin Chen [:bechen]]

(In reply to Sotaro Ikeda [:sotaro] from comment #11)
> Comment on attachment 8631452 [details] [diff] [review]
> 0001-Add-timestamp-for-the-last-decoded-sample.patch
> 
> Review of attachment 8631452 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Is this a patch for emulator-kk? It uses SoftAAC2.cpp of kk aosp. The code
> seems to have a defect related to EOS handling. And the problem seems to be
> fixed by kk caf code. Can's we use caf's fix, can we? It seems stable fix .
> 

Yes, I believe we can just use the caf's fix. The only different is that I add one more line |mNumSamplesOutput += mStreamInfo->frameSize;|, but I think it is fine without this line.

Comment 13

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8629859 [details] [diff] [review]
bug-1178682.v01.patch

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

::: dom/media/platforms/gonk/GonkAudioDecoderManager.cpp
@@ +163,5 @@
>      // quoted from Android's AwesomePlayer.cpp
>      ReleaseAudioBuffer();
>      return NS_ERROR_NOT_AVAILABLE;
>    }
>  

Can we add assert here? I want to prevent a risk of masking another bugs. Fix to SoftAAC2 should fix the test failure of this bug.

Comment 14

[Sotaro Ikeda [:sotaro]]

(In reply to Benjamin Chen [:bechen] from comment #12)
> 
> Yes, I believe we can just use the caf's fix. The only different is that I
> add one more line |mNumSamplesOutput += mStreamInfo->frameSize;|, but I
> think it is fine without this line.

SoftAAC2 bug seems to exist only on KK aosp. Therefore, I do not care which we are going to use. Which fix do you want to check in? attachment 8629859 [details] [diff] [review] or caf?

Comment 15

[Benjamin Chen [:bechen]]

(In reply to Sotaro Ikeda [:sotaro] from comment #14)
> (In reply to Benjamin Chen [:bechen] from comment #12)
> > 
> > Yes, I believe we can just use the caf's fix. The only different is that I
> > add one more line |mNumSamplesOutput += mStreamInfo->frameSize;|, but I
> > think it is fine without this line.
> 
> SoftAAC2 bug seems to exist only on KK aosp. Therefore, I do not care which
> we are going to use. Which fix do you want to check in? attachment 8629859 [details] [diff] [review]
> [details] [diff] [review] or caf?

I want check in both caf patch and our gecko patch.
I'll make a pull request for caf patch soon.

Comment 16

[Benjamin Chen [:bechen]]

Attachment: Add timestamp at the last decoded sample.

Comment 17

[Benjamin Chen [:bechen]]

Attachment: bug-1178682.v01.patch

Add assertion.

Comment 18

[Benjamin Chen [:bechen]]

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

Comment 19

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8b4de2466c80

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8b4de2466c80

Comment 21

[Benjamin Chen [:bechen]]

Hi sheriff:
Could you  please help to merge the pull request Attachment 8635141 [details] ?

Comment 22

[Carsten Book [:Tomcat]]

(In reply to Benjamin Chen [:bechen] from comment #21)
> Hi sheriff:
> Could you  please help to merge the pull request Attachment 8635141 [details]
> [details] ?

done https://github.com/mozilla-b2g/platform_frameworks_av/commit/f313503b5c91aaa6fcf962d4ec9bf260e0c00bf1