MediaCodecReader should handle EndOfStream of output data properly.

RESOLVED FIXED in 2.1 S2 (15aug)

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: brsun, Assigned: bechen)

Tracking

unspecified
2.1 S2 (15aug)
ARM
Gonk (Firefox OS)
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(feature-b2g:2.1)

Details

Attachments

(1 attachment, 2 obsolete attachments)

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.
Assignee: nobody → bechen
Posted patch bug-1043900.patch (obsolete) — Splinter Review
Attachment #8463284 - Flags: feedback?(brsun)
Blocks: 1033912
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?
Attachment #8463284 - Flags: feedback?(brsun) → feedback+
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.
Posted patch bug-1043900.patch (obsolete) — Splinter Review
Attachment #8463284 - Attachment is obsolete: true
Attachment #8466033 - Flags: review?(cpearce)
Attachment #8466033 - Flags: review?(cpearce) → review+
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.
Target Milestone: --- → 2.1 S2 (15aug)
feature-b2g: --- → 2.1
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.
Keywords: checkin-needed
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?
Flags: in-testsuite?
Keywords: checkin-needed
(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.
https://hg.mozilla.org/mozilla-central/rev/85fa5b0c166f
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Blocks: 1059613
You need to log in before you can comment on or make changes to this bug.