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)
Tracking
(tracking-b2g:backlog)
People
(Reporter: ethan, Assigned: bechen)
References
Details
(Whiteboard: [p=2])
Attachments
(1 file, 3 obsolete files)
6.75 KB,
patch
|
bechen
:
review+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Updated•11 years ago
|
Assignee: nobody → ettseng
Updated•11 years ago
|
blocking-b2g: --- → backlog
Reporter | ||
Comment 1•11 years ago
|
||
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()
Reporter | ||
Comment 2•11 years ago
|
||
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.
Reporter | ||
Comment 3•11 years ago
|
||
Assign to Benjamin since he has deeper insight of this part.
Assignee: ettseng → bechen
Assignee | ||
Comment 4•11 years ago
|
||
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
Assignee | ||
Comment 5•11 years ago
|
||
Hi Ethan:
Kindly try this patch, thanks.
Attachment #8414985 -
Flags: feedback?(ettseng)
Reporter | ||
Comment 6•11 years ago
|
||
(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
Reporter | ||
Updated•11 years ago
|
Attachment #8414985 -
Flags: feedback?(ettseng) → feedback+
Reporter | ||
Comment 7•11 years ago
|
||
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.
Reporter | ||
Comment 9•11 years ago
|
||
Hi Benjamin,
Do we have remaining issues of your patch?
How about pushing it into the review process?
Assignee | ||
Comment 10•11 years ago
|
||
fix nits.
Attachment #8414985 -
Attachment is obsolete: true
Attachment #8418633 -
Flags: review?(sworkman)
Comment 11•11 years ago
|
||
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+
Assignee | ||
Comment 12•11 years ago
|
||
r=sworkman, r=ethan
try server: https://tbpl.mozilla.org/?tree=Try&rev=b172004a5543
Attachment #8418633 -
Attachment is obsolete: true
Attachment #8419186 -
Flags: review+
Assignee | ||
Comment 13•11 years ago
|
||
change "r=ethan" to "r=ettseng"
Attachment #8419186 -
Attachment is obsolete: true
Attachment #8419212 -
Flags: review+
Assignee | ||
Updated•11 years ago
|
Keywords: checkin-needed
Reporter | ||
Updated•11 years ago
|
Whiteboard: [p=2]
Target Milestone: --- → 2.0 S2 (23may)
Comment 14•11 years ago
|
||
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
Comment 15•11 years ago
|
||
Keywords: checkin-needed
Comment 16•11 years ago
|
||
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: 2.0 S2 (23may) → 2.0 S1 (9may)
Comment 17•11 years ago
|
||
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
Updated•10 years ago
|
blocking-b2g: backlog → ---
tracking-b2g:
--- → backlog
You need to log in
before you can comment on or make changes to this bug.
Description
•