1043900 - MediaCodecReader should handle EndOfStream of output data properly.

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Bruce Sun [:brsun]]

Currently MediaCodecReader treats itself as EOS if input data from MediaSource encounters EOS. As a result, media files always stop earlier than the real end time. MediaCodecReader should consider EOS of output data from MediaCodec properly to have a more precise EOS condition.

Comment 1

[Benjamin Chen [:bechen]]

Attachment: bug-1043900.patch

Comment 2

[Bruce Sun [:brsun]]

Comment on attachment 8463284 [details] [diff] [review]
bug-1043900.patch

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

It looks good to me.

::: content/media/omx/MediaCodecReader.cpp
@@ +1066,3 @@
>    }
>  
>    while (status == OK || status == INFO_OUTPUT_BUFFERS_CHANGED || status == -EAGAIN) {

How about moving |if (info.mFlags & MediaCodec::BUFFER_FLAG_EOS) {...}| conditions into this while-loop to reduce duplicated codes?

Comment 3

[Bruce Sun [:brsun]]

Comment on attachment 8463284 [details] [diff] [review]
bug-1043900.patch

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

::: content/media/omx/MediaCodecReader.cpp
@@ +1061,5 @@
> +    if (info.mFlags & MediaCodec::BUFFER_FLAG_EOS) {
> +      aBuffer = info;
> +      aBuffer.mBuffer = aTrack.mOutputBuffers[info.mIndex];
> +      return ERROR_END_OF_STREAM;
> +    }

I mean this condition.

Comment 4

[Benjamin Chen [:bechen]]

Attachment: bug-1043900.patch

Comment 5

[Kai-Chih Hu [:khu]]

This got review+ already. Therefore, I think we can mark this bug's target milestone = sprint 2. If it's not true, please feel free to change the target milestone. Thanks.

Comment 6

[Benjamin Chen [:bechen]]

Attachment: bug-1043900.patch

r=cpearce
tryserver:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=e2da7d2f7ec0

Comment 7

[Benjamin Chen [:bechen]]

The try server results are most green in addition to the patch but also enable MediaCodec preference.
These orange fails seems not relative to MediaCodec.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/85fa5b0c166f

Do we have sufficient test coverage for this already? If not, can we add a test?

Comment 9

[Benjamin Chen [:bechen]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #8)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/85fa5b0c166f
> 
> Do we have sufficient test coverage for this already? If not, can we add a
> test?

If current B2G platform already have such testcases (test playback ended in this case), I think the test coverage is ok once we enable the "MediaCodec preference". Because we don't add any new functionality, we just replace OMXCodec by MediaCodec.

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/85fa5b0c166f