Closed
Bug 1080461
Opened 10 years ago
Closed 9 years ago
[RTSP] Live stream does not play automatically
Categories
(Firefox OS Graveyard :: RTSP, defect)
Tracking
(firefox36 fixed, firefox37 fixed)
RESOLVED
FIXED
2.2 S3 (9jan)
People
(Reporter: jhao, Assigned: jhao, Mentored)
References
Details
Attachments
(2 files, 7 obsolete files)
7.07 KB,
patch
|
jhao
:
review+
Sylvestre
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
2.41 KB,
patch
|
jhao
:
review+
|
Details | Diff | Splinter Review |
After opening a link of RTSP live stream, the player stays in paused state instead of playing automatically. User needs to press play button to play.
Comment 1•10 years ago
|
||
Hello, I would like to work on this bug, can you please provide more info, and is it possible to solve this bug without owning an actual firefox os device?
Comment 2•10 years ago
|
||
(In reply to Gaurav Mittal (gauravmittal1995) from comment #1) > Hello, I would like to work on this bug, can you please provide more info, > and is it possible to solve this bug without owning an actual firefox os > device? Hi Gaurav, We are welcome to have your help! RTSP streaming on Firefox OS supports only B2G platform for now. If you want to work on this bug, you must have a FFOS device. For now, our reference phone is Flame. If you are still interested in it, I think you can reach someone to get you a reference phone, though I am not sure. Howie, can you help on this?
Flags: needinfo?(hochang)
Comment 3•10 years ago
|
||
Hi Gaurav, you can order the reference device Flame online https://developer.mozilla.org/en-US/Firefox_OS/Developer_phone_guide/Flame. Let us know if you still want to continue to work on this bug, thanks.
Flags: needinfo?(hochang)
Assignee | ||
Comment 4•10 years ago
|
||
(In reply to Gaurav Mittal (gauravmittal1995) from comment #1) > Hello, I would like to work on this bug, can you please provide more info, > and is it possible to solve this bug without owning an actual firefox os > device? Hi Gaurav, Have you started on this bug or do you still want to work on it? If not, I'll take it.
Flags: needinfo?(gauravmittal1995)
Assignee | ||
Comment 5•9 years ago
|
||
I've compared live streams with normal streams. It seems that in live streams the mReadyState in HTMLMediaElement is never changed to HAVE_ENOUGH_DATA because in MediaDecoder::UpdateReadyStateForData(), the frameStatus from MediaDecoderStateMachine::GetNextFrameStatus() is always NEXT_FRAME_UNAVAILABLE. The reason is that HaveNextFrameData() always returns false because HasFutureAudio() is always false, and that's because AudioDecodedUsecs() is always zero. Looking deeper, the reason is that the AudioQueue().Duration() is always zero. I'm still looking into the cause.
Flags: needinfo?(gauravmittal1995)
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → jhao
Assignee | ||
Comment 6•9 years ago
|
||
This patch fixes the problem where HTMLMediaElement never HAVE_ENOUGH_DATA. The autoplay is still not kicked because in the end of HTMLMediaElement::UpdateReadyForChange(), this check fails > MediaDecoder::Statistics stats = mDecoder->GetStatistics(); > if (stats.mTotalBytes < 0 ? stats.mDownloadRateReliable > : stats.mTotalBytes == stats.mDownloadPosition || > mDecoder->CanPlayThrough()) > { > ChangeReadyState(nsIDOMHTMLMediaElement::HAVE_ENOUGH_DATA); > return; > } because for RTSP live streams, the mTotalBytes is -1, and mDownloadRateReliable is always false. That's why Bug 1111518 is filed to make some changes to this part and CanPlayThrough.
Assignee | ||
Comment 7•9 years ago
|
||
In order to make MediaDecoder::CanPlayThrough() know that it's decoding an live stream, the first thought was to use mDecoderStateMachine->mScheduler->IsRealTime(). However, this won't compile because mScheduler is a protected member. So, I added a public method IsRealTime() for MediaDecoderStateMachine returning mScheduler->IsRealTime(). I also changed all previous use of mScheduler->IsRealTime() to the newly implemented IsRealTime().
Attachment #8537670 -
Flags: review?(bechen)
Assignee | ||
Comment 8•9 years ago
|
||
The patch in Bug 1111518 is "r+". With that and the previous patch about MediaDecoderStateMachine::IsRealTime(), we can now add a check in MediaDecoder::CanPlayThrough() to make it return true for live streams.
Attachment #8537606 -
Attachment is obsolete: true
Attachment #8537693 -
Flags: review?(bechen)
Updated•9 years ago
|
Attachment #8537693 -
Flags: review?(bechen) → review+
Updated•9 years ago
|
Attachment #8537670 -
Flags: review?(jwwang)
Attachment #8537670 -
Flags: review?(bechen)
Attachment #8537670 -
Flags: review+
Updated•9 years ago
|
Attachment #8537670 -
Flags: review?(jwwang) → review+
Comment 9•9 years ago
|
||
Comment on attachment 8537693 [details] [diff] [review] (Part 2) Fix RTSP autoplay, check IsRealTime() in CanPlayThrough() Review of attachment 8537693 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/media/MediaDecoder.cpp @@ +1565,5 @@ > > bool MediaDecoder::CanPlayThrough() > { > Statistics stats = GetStatistics(); > + if (mDecoderStateMachine->IsRealTime() || Can you explain the reasonale behind this? ::: dom/media/MediaDecoderStateMachine.cpp @@ +266,5 @@ > } > > bool MediaDecoderStateMachine::HaveNextFrameData() { > AssertCurrentThreadInMonitor(); > + return IsRealTime() || It looks like cheating to me. Can't we rely on the old rules?
Assignee | ||
Comment 10•9 years ago
|
||
(In reply to JW Wang [:jwwang] from comment #9) Thanks for the feedback! > Comment on attachment 8537693 [details] [diff] [review] > (Part 2) Fix RTSP autoplay, check IsRealTime() in CanPlayThrough() > > Review of attachment 8537693 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/media/MediaDecoder.cpp > @@ +1565,5 @@ > > > > bool MediaDecoder::CanPlayThrough() > > { > > Statistics stats = GetStatistics(); > > + if (mDecoderStateMachine->IsRealTime() || > > Can you explain the reasonale behind this? This was to make CanPlayThrough return true on RTSP live stream. On second thoughts, maybe I should implement RtspMediaResource::GetDownloadRate() to make mDownloadRateReliable return true, instead of changing the logic in CanPlayThrough. > ::: dom/media/MediaDecoderStateMachine.cpp > @@ +266,5 @@ > > } > > > > bool MediaDecoderStateMachine::HaveNextFrameData() { > > AssertCurrentThreadInMonitor(); > > + return IsRealTime() || > > It looks like cheating to me. Can't we rely on the old rules? I will also think about if this one is necessary.
Assignee | ||
Comment 11•9 years ago
|
||
(In reply to JW Wang [:jwwang] from comment #9) > Comment on attachment 8537693 [details] [diff] [review] > (Part 2) Fix RTSP autoplay, check IsRealTime() in CanPlayThrough() > > Review of attachment 8537693 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/media/MediaDecoder.cpp > @@ +1565,5 @@ > > > > bool MediaDecoder::CanPlayThrough() > > { > > Statistics stats = GetStatistics(); > > + if (mDecoderStateMachine->IsRealTime() || > > Can you explain the reasonale behind this? Currently, RTSP doesn't reply correct statistics, so for RTSP live streams, CanPlayThrough() never returns true. This check is kind of a workaround. To solve this legitimately, we should implement RTSPMediaResource::GetLength() and GetDownloadRate() to make the statistics correct. > ::: dom/media/MediaDecoderStateMachine.cpp > @@ +266,5 @@ > > } > > > > bool MediaDecoderStateMachine::HaveNextFrameData() { > > AssertCurrentThreadInMonitor(); > > + return IsRealTime() || > > It looks like cheating to me. Can't we rely on the old rules? This one is because for RTSP live stream, HasFutureAudio() is always false. The reason is that in MediaDecoderStateMachine::DecodeFirstFrame(), we didn't request data for live streams. According to Benjamin, live streams doesn't guarantee sending us data, so we can't do that. I can't think of other solutions. Do you have any suggestions?
Assignee | ||
Comment 12•9 years ago
|
||
In offline discussions, JW pointed out that the decoder should request data for live streams, and Benjamin and I later found out that the audio queue was not empty, but the duration was still zero because the duration is implemented as the difference of mTime. Since live streams have no time stamps, their mTime are fixed dummy values, so the difference is zero. Benjamin suggested to use frame count instead of duration in audio case.
Assignee | ||
Comment 13•9 years ago
|
||
In CanPlayThrough(), RTSP live streams currently doesn't provide correct byte statistics, so it's kind of a workaround. For real-time streams (currently only RTSP), we assume CanPlayThrough() is true to kick up autoplay. Since RTSP already leaves a 3-second playout delay, this shouldn't cause severe lagging. In AudioDecodedUsecs(), since RTSP live streams have no time stamps, AudioQueue().Duration() is not accurate, so we calculate it from frame counts.
Attachment #8537693 -
Attachment is obsolete: true
Attachment #8542031 -
Flags: review?(jwwang)
Attachment #8542031 -
Flags: review?(bechen)
Assignee | ||
Comment 14•9 years ago
|
||
Attachment #8542031 -
Attachment is obsolete: true
Attachment #8542031 -
Flags: review?(jwwang)
Attachment #8542031 -
Flags: review?(bechen)
Attachment #8542032 -
Flags: review?(jwwang)
Attachment #8542032 -
Flags: review?(bechen)
Comment 15•9 years ago
|
||
Comment on attachment 8542032 [details] [diff] [review] (Part 2) Fix RTSP autoplay, check IsRealTime() in CanPlayThrough() Review of attachment 8542032 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/media/MediaDecoder.cpp @@ +1561,5 @@ > > bool MediaDecoder::CanPlayThrough() > { > Statistics stats = GetStatistics(); > + if (mDecoderStateMachine->IsRealTime() || assert or check mDecoderStateMachine is not null. ::: dom/media/MediaDecoderStateMachine.cpp @@ +1927,5 @@ > // The amount of audio we have decoded is the amount of audio data we've > // already decoded and pushed to the hardware, plus the amount of audio > // data waiting to be pushed to the hardware. > int64_t pushed = (mAudioEndTime != -1) ? (mAudioEndTime - GetMediaTime()) : 0; > + if (IsRealTime()) 1. if (IsRealTime()) { <-- always put braces. 2. add comments for why the duration is calculated in a different way for a live stream. 3. in the long term, we still need to fix bug 1114434 so AudioQueue().Duration() shall return a correct value for live streams.
Attachment #8542032 -
Flags: review?(jwwang) → review+
Assignee | ||
Comment 16•9 years ago
|
||
Modified as JW suggested.
Attachment #8542032 -
Attachment is obsolete: true
Attachment #8542032 -
Flags: review?(bechen)
Attachment #8542069 -
Flags: review?(bechen)
Updated•9 years ago
|
Attachment #8542069 -
Flags: review?(bechen) → review+
Assignee | ||
Comment 17•9 years ago
|
||
Revise commit message
Attachment #8537670 -
Attachment is obsolete: true
Attachment #8542388 -
Flags: review+
Assignee | ||
Comment 18•9 years ago
|
||
Try server: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a9630d077894 Due to dependency, Part 1 should be checked-in before Part 2.
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 20•9 years ago
|
||
Rebased
Attachment #8542388 -
Attachment is obsolete: true
Attachment #8544312 -
Flags: review+
Assignee | ||
Comment 21•9 years ago
|
||
Rebased
Attachment #8542069 -
Attachment is obsolete: true
Attachment #8544313 -
Flags: review+
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Comment 22•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/c5075104363d https://hg.mozilla.org/integration/mozilla-inbound/rev/2b1854f193f9
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/c5075104363d https://hg.mozilla.org/mozilla-central/rev/2b1854f193f9
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.2 S3 (9jan)
Comment 24•9 years ago
|
||
Comment on attachment 8544312 [details] [diff] [review] (Part 1) Add IsRealTime() in MediaDecoderStateMachine Both patches landed on aurora with a=mse pre-approval. These are not related to MSE, but I want to reduce the diff in MediaDecoder between 36 and 37 in support of debugging and any MSE-specific backports in the beta period. https://hg.mozilla.org/releases/mozilla-aurora/rev/842beaf6b3df https://hg.mozilla.org/releases/mozilla-aurora/rev/5426fded351c Part 1 is minimal risk: it just adds a new method. Part 2 is moderate risk: it adds additional criteria for entering CanPlayThrough. Pre-flight try push on top of Aurora: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=23d216f9581f
Attachment #8544312 -
Flags: approval-mozilla-aurora?
Updated•9 years ago
|
status-firefox36:
--- → fixed
status-firefox37:
--- → fixed
Updated•9 years ago
|
Attachment #8544312 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in
before you can comment on or make changes to this bug.
Description
•