Closed Bug 1000195 Opened 11 years ago Closed 11 years ago

[RTSP] Follow-up of 992568 - RTSP audio does not auto-play

Categories

(Firefox OS Graveyard :: RTSP, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(tracking-b2g:backlog)

VERIFIED FIXED
2.0 S1 (9may)
tracking-b2g backlog

People

(Reporter: ethan, Assigned: bechen)

References

Details

(Whiteboard: [p=2])

Attachments

(1 file, 3 obsolete files)

This is a follow-up of bug 992568. When we open an RTSP link, the built-in media player of B2G browser would automatically play if the source is an RTSP video media. This behavior also aligns to HTTP streaming. However, it doesn't automatically play for RTSP audio-only media.
Assignee: nobody → ettseng
blocking-b2g: --- → backlog
When we click an audio-only RTSP link, RtspController::Pause() would be called immediately after Play(). But this wouldn't happen for an RTSP video media. Play() call sequence: ... -> MediaDecoderStateMachine::DecodeMetadata() -> RtspOmxReader::ReadMetadata() -> RtspOmxReader::SetActive() -> ... -> RtspController::Play() Pause() call sequence: ... -> MediaDecoderStateMachine::SetReaderIdle() -> RtspOmxReader::SetIdle() -> ... -> RtspController::Pause()
The divergence happens in MediaDecoderStateMachine::DispatchDecodeTasksIfNeeded(). http://dxr.mozilla.org/mozilla-central/source/content/media/MediaDecoderStateMachine.cpp#1562 For the case of RTSP audio, needToDecodeAudio would become false soon after Play, and consequently calls SetReaderIdle() and Pause(). This behavior doesn't align to HTTP streaming for audio media.
Assign to Benjamin since he has deeper insight of this part.
Assignee: ettseng → bechen
Blocks: 996765
For now, the auto-play is kickoff by MediaCache. And the rtsp doesn't leverage MediaCache... #0 mozilla::MediaDecoder::Play (this=0x430d3800) at /home/benjamin/Documents/hg/mozilla-central/content/media/MediaDecoder.cpp:596 #1 0x40f47f5e in mozilla::dom::HTMLMediaElement::CheckAutoplayDataReady (this=0x430fc640) at /home/benjamin/Documents/hg/mozilla-central/content/html/content/src/HTMLMediaElement.cpp:3217 #2 0x40f485ca in mozilla::dom::HTMLMediaElement::NotifySuspendedByCache (this=0x430d3800, aIsSuspended=false) at /home/benjamin/Documents/hg/mozilla-central/content/html/content/src/HTMLMediaElement.cpp:3030 #3 0x40f80658 in mozilla::MediaDecoder::NotifySuspendedStatusChanged (this=0x430d3800) at /home/benjamin/Documents/hg/mozilla-central/content/media/MediaDecoder.cpp:976 #4 0x40f8916c in mozilla::ChannelMediaResource::CacheClientSuspend (this=0x4036bc00) at /home/benjamin/Documents/hg/mozilla-central/content/media/MediaResource.cpp:1066 #5 0x40f7fc90 in mozilla::MediaCache::Update (this=0x430fadd0) at /home/benjamin/Documents/hg/mozilla-central/content/media/MediaCache.cpp:1353
Attached patch bug-1000195.patch (obsolete) — Splinter Review
Hi Ethan: Kindly try this patch, thanks.
Attachment #8414985 - Flags: feedback?(ettseng)
(In reply to Benjamin Chen [:bechen] from comment #5) > Created attachment 8414985 [details] [diff] [review] > bug-1000195.patch > Hi Ethan: > Kindly try this patch, thanks. Hi Benjamin, Good news! I applied your patch and performed manual tests a dozen of times. Every time RTSP audio can be auto-played. In the meanwhile, RTSP video is not affected, it can be auto-played as well. However, as you know, there is a difference between RTSP video and audio from the perspective of Controller's play/pause events. - RTSP video: play - RTSP audio: play-pause-play
Attachment #8414985 - Flags: feedback?(ettseng) → feedback+
Comment on attachment 8414985 [details] [diff] [review] bug-1000195.patch Review of attachment 8414985 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/html/content/src/HTMLMediaElement.cpp @@ +7,5 @@ > #include "mozilla/dom/HTMLMediaElement.h" > #include "mozilla/dom/HTMLMediaElementBinding.h" > #include "mozilla/dom/ElementInlines.h" > #include "mozilla/ArrayUtils.h" > #include "mozilla/MathAlgorithms.h" It seems there is no change here. ::: content/media/RtspMediaResource.cpp @@ +384,5 @@ > mListener->Revoke(); > } > } > > +void RtspMediaResource::SetSuspend(bool aIsSuspend){ nit: please add a space between the right parentheses and the left bracket. @@ +394,5 @@ > + aIsSuspend); > + NS_DispatchToMainThread(runnable); > +} > + > +void RtspMediaResource::NotifySuspend(bool aIsSuspend){ ditto.
Hi Benjamin, Do we have remaining issues of your patch? How about pushing it into the review process?
Attached patch bug-1000195.patch (obsolete) — Splinter Review
fix nits.
Attachment #8414985 - Attachment is obsolete: true
Attachment #8418633 - Flags: review?(sworkman)
Comment on attachment 8418633 [details] [diff] [review] bug-1000195.patch Review of attachment 8418633 [details] [diff] [review]: ----------------------------------------------------------------- Looks like Ethan has already looked over this, so r=me. Please add r=ettseng as well as r=sworkman to the commit message.
Attachment #8418633 - Flags: review?(sworkman) → review+
Attached patch bug-1000195.patch (obsolete) — Splinter Review
Attachment #8418633 - Attachment is obsolete: true
Attachment #8419186 - Flags: review+
change "r=ethan" to "r=ettseng"
Attachment #8419186 - Attachment is obsolete: true
Attachment #8419212 - Flags: review+
Keywords: checkin-needed
Whiteboard: [p=2]
Target Milestone: --- → 2.0 S2 (23may)
Benjamin, when you get a chance, please review the link below. We're trying to avoid infra backlogs and extraneous builds/tests on Try are a big part of it. Thanks! https://wiki.mozilla.org/Sheriffing/How:To:Recommended_Try_Practices
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: 2.0 S2 (23may) → 2.0 S1 (9may)
Verified it. Thanks. * Build information: (V2.0 - Flame) - Gaia a38a6a5c6fabc97dd16d5360632b5ac5c7e06241 - Gecko https://hg.mozilla.org/mozilla-central/rev/951e3a671279 - BuildID 20140604160202 - Version 32.0a1
Status: RESOLVED → VERIFIED
See Also: → 1080461
blocking-b2g: backlog → ---
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: