Closed Bug 1080461 Opened 8 years ago Closed 8 years ago

[RTSP] Live stream does not play automatically

Categories

(Firefox OS Graveyard :: RTSP, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(firefox36 fixed, firefox37 fixed)

RESOLVED FIXED
2.2 S3 (9jan)
Tracking Status
firefox36 --- fixed
firefox37 --- fixed

People

(Reporter: jhao, Assigned: jhao, Mentored)

References

Details

Attachments

(2 files, 7 obsolete files)

7.07 KB, patch
jhao
: review+
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.
Blocks: 1054171
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?
(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)
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)
See Also: → 1000195
(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)
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: nobody → jhao
Depends on: 1111518
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.
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)
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)
Attachment #8537693 - Flags: review?(bechen) → review+
Attachment #8537670 - Flags: review?(jwwang)
Attachment #8537670 - Flags: review?(bechen)
Attachment #8537670 - Flags: review+
Attachment #8537670 - Flags: review?(jwwang) → review+
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?
(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.
(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?
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.
Depends on: 1114434
No longer depends on: 1114434
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)
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 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+
Modified as JW suggested.
Attachment #8542032 - Attachment is obsolete: true
Attachment #8542032 - Flags: review?(bechen)
Attachment #8542069 - Flags: review?(bechen)
Attachment #8542069 - Flags: review?(bechen) → review+
Revise commit message
Attachment #8537670 - Attachment is obsolete: true
Attachment #8542388 - Flags: review+
Try server: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a9630d077894

Due to dependency, Part 1 should be checked-in before Part 2.
Keywords: checkin-needed
Needs rebasing.
Keywords: checkin-needed
Rebased
Attachment #8542388 - Attachment is obsolete: true
Attachment #8544312 - Flags: review+
Keywords: checkin-needed
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?
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.