Closed Bug 1154194 Opened 7 years ago Closed 6 years ago

MediaCodecReader: testcase test_bug448534.html failed.

Categories

(Core :: Audio/Video, defect)

x86_64
Windows 7
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox40 --- affected
firefox41 --- fixed

People

(Reporter: bechen, Assigned: bechen)

References

Details

Attachments

(2 files, 4 obsolete files)

STR: Enable MediaCodec preference, emulator-kk build, test_bug448534.html failed.
The test file small-shot.flac is a quite small file, the return value of FillCodecInputData() in EnsureCodecFormatParsed() is ERROR_END_OF_STREAM then return false.
Attached patch bug-1154194.v01.patch (obsolete) — Splinter Review
Attachment #8592142 - Flags: feedback?(brsun)
Comment on attachment 8592142 [details] [diff] [review]
bug-1154194.v01.patch

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

Hi Benjamin,

Thanks for catching this issue. Please kindly see my comments below.

::: dom/media/omx/MediaCodecReader.cpp
@@ +1896,5 @@
>  
>      status = FillCodecInputData(aTrack);
>      if (status == INFO_FORMAT_CHANGED) {
>        break;
> +    } else if (status == ERROR_END_OF_STREAM) {

It might be better to have some more information to explicitly express the intention of this empty block. How about adding |continue| in this block instead of leaving it empty?

One more question. What do you think if we combine the conditional checking of |INFO_FORMAT_CHANGED| and |ERROR_END_OF_STREAM| together, and simply break the while loop? Does breaking in this way make differences in practice comparing to adding one empty block for end-of-stream? Not a suggestion, it just comes from my curiosity.
Comment on attachment 8592142 [details] [diff] [review]
bug-1154194.v01.patch

Remove feedback? for the moment (refer to comment 3).
Attachment #8592142 - Flags: feedback?(brsun)
Attached patch bug-1154194.v01.patch (obsolete) — Splinter Review
Remove the check for FillCodecInputData the small file.
Attachment #8592142 - Attachment is obsolete: true
Attachment #8605680 - Flags: review?(sotaro.ikeda.g)
Attachment #8605680 - Flags: review?(sotaro.ikeda.g) → review+
I'm still working on the bug because the patch only resolve the small file issue, now I hit the |!mAudioTrack.mCodec->allocated()| when the testcase trying to play a mp3 file (small-shot.mp3).
See Also: → 1132832
Attached patch bug-1154194.part2.v01.patch (obsolete) — Splinter Review
Since bug 1132832 change the MediaCodecProxy's behavior, we need to call AskMediaCodecAndWait() for playback. And we don't need to handle "waiting video resource". So I remove the code about |VideoResourceListener|.
Attachment #8606900 - Flags: review?(sotaro.ikeda.g)
Blocks: 1165825
Comment on attachment 8606900 [details] [diff] [review]
bug-1154194.part2.v01.patch

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

Looks good! I quickly checked bug 1132832. bug 1132832 might not handle a case that video codec resource allocation could take very long time or infinite time though.
Attachment #8606900 - Flags: review?(sotaro.ikeda.g) → review+
r=sotaro
Attachment #8605680 - Attachment is obsolete: true
Attachment #8609211 - Flags: review+
Attached patch bug-1154194.part2.v01.patch (obsolete) — Splinter Review
r=sotaro

try server:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=11ff930b1e7a
Attachment #8606900 - Attachment is obsolete: true
Attachment #8609212 - Flags: review+
Keywords: checkin-needed
Part 2 failed to apply:

adding 1154194 to series file
renamed 1154194 -> bug-1154194.part2.v01.patch
applying bug-1154194.part2.v01.patch
patching file dom/media/omx/MediaCodecReader.cpp
Hunk #6 FAILED at 1869
1 out of 6 hunks FAILED -- saving rejects to file dom/media/omx/MediaCodecReader.cpp.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and refresh bug-1154194.part2.v01.patch

could you take a look, thanks!
Flags: needinfo?(bechen)
Keywords: checkin-needed
Rebase.
Attachment #8609212 - Attachment is obsolete: true
Flags: needinfo?(bechen)
Attachment #8610444 - Flags: review+
Keywords: checkin-needed
bechen, is there a specific reason to use AskMediaCodecAndWait()? I re-checked the code, attachment 8610444 [details] [diff] [review] seems better revert to old code, because AskMediaCodecAndWait() could block thread for a very long long time.
Flags: needinfo?(bechen)
Blocks: 1168456
I am going to revert AskMediaCodecAndWait() as part of Bug 1168456.
Depends on: 1168531
No longer depends on: 1168531
No longer blocks: 1168456
Depends on: 1168456
No longer depends on: 1168456
Depends on: 1168531
(In reply to Sotaro Ikeda [:sotaro] from comment #14)
> bechen, is there a specific reason to use AskMediaCodecAndWait()? I
> re-checked the code, attachment 8610444 [details] [diff] [review] seems
> better revert to old code, because AskMediaCodecAndWait() could block thread
> for a very long long time.

No, I just want to fix the MediaCodecReader playback as soon as possible. So that I can keep fixing other MediaCodecReader's problem on emulator-kk build.
Flags: needinfo?(bechen)
Thanks, Bug 1165825 and Bug 1168456 is going to address AskMediaCodecAndWait()'s problem.
You need to log in before you can comment on or make changes to this bug.