Closed Bug 1078125 Opened 10 years ago Closed 10 years ago

MediaCodecReader: RTSP stream can not playback.

Categories

(Firefox OS Graveyard :: RTSP, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
2.1 S9 (21Nov)

People

(Reporter: bechen, Assigned: bechen)

Details

Attachments

(1 file, 1 obsolete file)

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
Attached patch bug-1078125.v01.patch (obsolete) — Splinter Review
Attachment #8511846 - Flags: review?(ettseng)
Attachment #8511846 - Flags: review?(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?
Flags: needinfo?(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.
Flags: needinfo?(bechen)
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?
Attachment #8511846 - Flags: review?(brsun) → review?(cpearce)
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?
Attachment #8511846 - Flags: review?(cpearce) → review+
(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 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.
Attachment #8511846 - Flags: review?(ettseng) → review+
r=cpearce, r=ethan
Attachment #8511846 - Attachment is obsolete: true
Attachment #8525744 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/aa46b3ca0f9f
Status: NEW → RESOLVED
Closed: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 2.1 S9 (21Nov)
You need to log in before you can comment on or make changes to this bug.