1078125 - MediaCodecReader: RTSP stream can not playback.

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RTSP, defect)

Description

[Benjamin Chen [:bechen]]

Because MediaCodecReader::ReadMetadata will try to read a decoded audio sample to update audio information by UpdateAudioInfo(), RTSP must start streaming before ReadMetadata, otherwise the stream will be blocked.

And remove
http://dxr.mozilla.org/mozilla-central/source/content/media/omx/MediaCodecReader.cpp?from=MediacodecReader.cpp#668

Comment 1

[Benjamin Chen [:bechen]]

Attachment: bug-1078125.v01.patch

Comment 2

[Bruce Sun [:brsun]]

Comment on attachment 8511846 [details] [diff] [review]
bug-1078125.v01.patch

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

The logic to trigger SetIdle() is a little bit different from RtspOmxReader:
 - http://dxr.mozilla.org/mozilla-central/source/dom/media/omx/RtspOmxReader.cpp#93

Which one is more promising?

Comment 3

[Benjamin Chen [:bechen]]

(In reply to Bruce Sun [:brsun] from comment #2)
> Comment on attachment 8511846 [details] [diff] [review]
> bug-1078125.v01.patch
> 
> Review of attachment 8511846 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The logic to trigger SetIdle() is a little bit different from RtspOmxReader:
>  -
> http://dxr.mozilla.org/mozilla-central/source/dom/media/omx/RtspOmxReader.
> cpp#93
> 
> Which one is more promising?

I think this one is better because we can not presume the "auto play" attribute for the MediaElement is always true. So we had better to call SetIdle() after ReadMetadata.

Comment 4

[Bruce Sun [:brsun]]

Comment on attachment 8511846 [details] [diff] [review]
bug-1078125.v01.patch

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

Seems good.

cpearce: what do you think?

Comment 5

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8511846 [details] [diff] [review]
bug-1078125.v01.patch

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

Fine with me.

Does this also need to happen with the MediaOmxReader and MediaCodecReader?

Comment 6

[Bruce Sun [:brsun]]

(In reply to Chris Pearce (:cpearce) from comment #5)
> Does this also need to happen with the MediaOmxReader and MediaCodecReader?

This should be benefits to call SetIdle() after ReadMetadata() in MediaOmxReader as well since it can save power consumption on the platform that supports OMXCodec::pause().

Comment 7

[Ethan Tseng [:ethan]]

Comment on attachment 8511846 [details] [diff] [review]
bug-1078125.v01.patch

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

I am fine with the change.
But I have a concern about auto-play. Stated inline.

::: dom/media/omx/RtspMediaCodecReader.cpp
@@ +98,2 @@
>    nsresult rv = MediaCodecReader::ReadMetadata(aInfo, aTags);
> +  SetIdle();

So, it will pause RTSP stream after reading metadata.
Can we ensure it will be played again if auto-play attribute is enabled?
I remember we had a problem with auto-playing in RtspOmxReader.cpp.

Comment 8

[Benjamin Chen [:bechen]]

try server:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=c8f42830ecde

Comment 9

[Benjamin Chen [:bechen]]

Attachment: bug-1078125.v01.patch

r=cpearce, r=ethan

Comment 10

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

https://hg.mozilla.org/mozilla-central/rev/aa46b3ca0f9f