If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

MediaCodecReader should handle EndOfStream of output data properly.

RESOLVED FIXED in 2.1 S2 (15aug)

Status

()

Core
Audio/Video
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: brsun, Assigned: bechen)

Tracking

(Blocks: 2 bugs)

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)

(Reporter)

Description

3 years ago
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)

Updated

3 years ago
Assignee: nobody → bechen
(Assignee)

Comment 1

3 years ago
Created attachment 8463284 [details] [diff] [review]
bug-1043900.patch
Attachment #8463284 - Flags: feedback?(brsun)
(Assignee)

Updated

3 years ago
Blocks: 1033912
(Reporter)

Comment 2

3 years ago
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+
(Reporter)

Comment 3

3 years ago
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.
(Assignee)

Comment 4

3 years ago
Created attachment 8466033 [details] [diff] [review]
bug-1043900.patch
Attachment #8463284 - Attachment is obsolete: true
Attachment #8466033 - Flags: review?(cpearce)
Attachment #8466033 - Flags: review?(cpearce) → review+

Comment 5

3 years ago
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)

Updated

3 years ago
feature-b2g: --- → 2.1
(Assignee)

Comment 6

3 years ago
Created attachment 8466919 [details] [diff] [review]
bug-1043900.patch

r=cpearce
tryserver:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=e2da7d2f7ec0
Attachment #8466033 - Attachment is obsolete: true
Attachment #8466919 - Flags: review+
(Assignee)

Comment 7

3 years ago
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
(Assignee)

Comment 9

3 years ago
(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
Last Resolved: 3 years ago
Resolution: --- → FIXED
(Assignee)

Updated

3 years ago
Blocks: 1059613
You need to log in before you can comment on or make changes to this bug.