1119681 - [RTSP] add time stamps to RTSP live streams

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RTSP, defect)

Comment 1

[Jonathan Hao (inactive) [:jhao]]

At first, we weren't sure if RTP timestamps of real time streams are reliable. However, it turns out doing well and significantly improves the performance. The difference is very obvious on the following stream rtsp://78.109.84.47/public/flux15

There's already a wip patch in bug 1080467. But it still needs some fixes to make playout delay work. Also, we should take care of the discontinuity when live streams are paused then play again.

Comment 2

[Jonathan Hao (inactive) [:jhao]]

Attachment: Adding timestamps

As Benjamin said in https://bugzilla.mozilla.org/show_bug.cgi?id=1080467#c3 if we have valid timestamps, we should pretend it's not a live stream to make MediaDecoderStateMachine apply playout delay.

However, we are not sure if we may encounter live streams with invalid RTP timestamps, so we keep two private boolean values now:
1. mHasTimeStamps - which is currently always true. This value will be returned by public method IsRealTime()
2. mIsLiveStream - which will be used to determine if the stream has end time, or if the stream is seekable.

I haven't dealt with the problem with pausing and seeking. I'm just asking Benjamin's if the above logic and naming is OK, and if the comments are enough.

Comment 3

[Benjamin Chen [:bechen]]

Comment on attachment 8546445 [details] [diff] [review]
Adding timestamps

Review of attachment 8546445 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/RtspMediaResource.h
@@ +251,5 @@
>    bool mIsConnected;
>    // live stream
> +  bool mIsLiveStream;
> +  // has rtp timestamps
> +  bool mHasTimestamps;

Each RTP packet has only one timestamp, right?
So we should name it timestamp instead timestamps?

Comment 4

[Jonathan Hao (inactive) [:jhao]]

After testing, it seems that some live streams server doesn't accept PAUSE request. In order not to keep receiving data after pausing, we have to close the entire session. I will try to put the logic in RTSPSource or RTSPConnectionHandler to avoid drastic changes to DecoderStateMachine.

Comment 5

[Jonathan Hao (inactive) [:jhao]]

Attachment: Send teardown if paused

In this patch, if user press pause, RTSPSource will disconnect with the server, and then restart a new session. In media decoder, I dispatch a playbackended() to make it go to PLAY_STATE_ENDED.

However, there's a timing issue. If the user press pause and immediately plays again, the new connection may not be done yet, and the decoder starts decoding prematurely.

Perhaps making it go to ENDED state is not a good idea, instead we could make it go to the beginning or the DORMANT state.

Comment 6

[Jonathan Hao (inactive) [:jhao]]

Attachment: Adding timestamps

This patch uses RTP-time as timestamps for live streams. Changes are mostly in RtspMediaResource as I asked Benjamin for feedbacks. Since there're timestamps now, MediaDecoderStateMachine can treat live streams just as non-live streams, so play-out delay can be done.

Besides, there's the problem that live streams server can't handle PAUSE requests. My solution is that in RTSPSource, if |pause()| is called, |stop()| is performed instead, and a flag mDisconnectedToPauseLiveStream is set. With this flag on, we don't report disconnection to the controller, thus letting Decoder go to PAUSED state. Later when RTSPSource::play() is called, a new session is started. Due to timing issues, a play() is added at the end of RTSPSource::onConnected(), to ensure it starts playing after successfully connected to the server.

I deliberately avoid touching MediaDecoder and MediaDecoderStateMachine so that I don't break its logic.

Comment 7

[Jonathan Hao (inactive) [:jhao]]

With this patch, the performance will be great. But if user paused and play again, the play-out delay will be gone. The performance will be bad for a few seconds, but will recover afterwards.

Comment 8

[Ethan Tseng [:ethan]]

Comment on attachment 8551186 [details] [diff] [review]
Adding timestamps

Review of attachment 8551186 [details] [diff] [review]:
-----------------------------------------------------------------

Jonathan, thanks for writing this patch!

I have some comments and questions inline.
It seems this patch is based on some trial patch in your local mercurial queue.
(ETHAN_LOG() is included in the previous version of patch difference!).

Please clean up your MQ and rebase the patch.

::: dom/media/RtspMediaResource.cpp
@@ +509,5 @@
>      nsIChannel* aChannel, nsIURI* aURI, const nsACString& aContentType)
>    : BaseMediaResource(aDecoder, aChannel, aURI, aContentType)
>    , mIsConnected(false)
> +  , mIsLiveStream(false)
> +  , mHasTimestamps(true)

Is mHasTimestamps always true?
I don't see any logic to determine the value of this flag.

::: netwerk/protocol/rtsp/rtsp/RTSPSource.cpp
@@ +121,5 @@
>  {
>      LOGI("RTSPSource::pause()");
>      ETHAN_LOG("RTSPSource::pause()");
> +
> +    // live streams can't be paused, we have to disconnect now

Please capitalize the first letter and add a period in the end.

@@ +126,5 @@
> +    if (isLiveStream()) {
> +        mDisconnectedToPauseLiveStream = true;
> +        stop();
> +        return;
> +    }

Once paused for a live stream, mDisconnectedToPauseLiveStream becomes true
forever.
What would happen if we re-play the paused live stream and then a "real"
disconnection occurs (either stopped by decoder or caused by network error)?
Should we set mDisconnectedToPauseLiveStream as false in RTSPSource::play()?

@@ +665,5 @@
>  
>      ETHAN_LOG("RTSPSource::onConnected()");
>      mState = CONNECTED;
> +
> +    play();

Is this play added for the case of re-playing after pausing a live stream?
Please add comments to make it clear.
Furthermore, this could break the logic of auto-play setting of video element.
Maybe we should add some checks before executing play here.

@@ +685,5 @@
>      if (mDisconnectReplyID != 0) {
>          finishDisconnectIfPossible();
>      }
> +    // if the disconnection is caused by pausing live stream
> +    // do not report back to the controller

Capitalize the first letter and add comma and period.
// If the disconnection is caused by pausing live stream,
// do not report back to the controller.

@@ +764,3 @@
>      while (dequeueAccessUnit(info->mIsAudio, &accessUnit) == OK) {
>          ETHAN_LOG("dequeue");
>      }

This while() loop doesn't exist in current repository.
You have to rebase your patch by cleaning your local mercurial queue.

Comment 9

[Jonathan Hao (inactive) [:jhao]]

Attachment: Adding timestamps

I rebased the patch, and revised the comments. Also, I set mDisconnectedToPauseLiveStream to false at the beginning of RTSPSource::start().

As for HasTimeStamps always being true, it was Benjamin's concerns that maybe someday we'll find a live stream server sending us packets with invalid rtp-times, so we should preserve this flag. For now, it's always true.

Comment 10

[Ethan Tseng [:ethan]]

Comment on attachment 8551551 [details] [diff] [review]
Adding timestamps

Review of attachment 8551551 [details] [diff] [review]:
-----------------------------------------------------------------

I notice you removed the call to play() in RTSPSource::onConnected() in your previous patch.
Please make sure this is what you want.

The patch looks fine to me. Thanks.

Comment 11

[Jonathan Hao (inactive) [:jhao]]

Attachment: Adding timestamps

Hi Ethan,

Sorry, I overlooked your concern about the immediate |play()| after connected. It doesn't break the logic of autoplay because an HTML video tag with autoplay attribute false doesn't even setup the connection. However, Ethan told me offline that there's also a "preload" option. Although RTSP streams doesn't have cache yet, I'll set another flag |mPlayOnConnected| to determine if a connection is triggered by a play request from above.

Please check one more time. Thanks a lot!

Comment 12

[Ethan Tseng [:ethan]]

Comment on attachment 8552948 [details] [diff] [review]
Adding timestamps

Review of attachment 8552948 [details] [diff] [review]:
-----------------------------------------------------------------

Excellent! Adding mPlayOnConnected is much more clear. r=me

::: netwerk/protocol/rtsp/rtsp/RTSPSource.h
@@ +158,5 @@
> +    // in order to pretend pausing a live stream.
> +    bool mDisconnectedToPauseLiveStream;
> +
> +    bool mPlayOnConnected;
> +

Although this variable name is somewhat self-explained, I suggest it's still
better to add comment to explain its purpose.

Here's my suggestion:
// While performing a play operation, if the current state of RTSP connection
// is disconnected, we will start over establishing connection to the server.
// In this case (mPlayOnConnected = true), we have to perform play again when 
// onConnected, to ensure we complete the play operation.

Comment 13

[Jonathan Hao (inactive) [:jhao]]

Attachment: Adding timestamps

I added the suggested comment of Ethan, and modified the commit message.

Comment 14

[Jonathan Hao (inactive) [:jhao]]

Attachment: Adding timestamps

Modify comments again.

Comment 15

[Jonathan Hao (inactive) [:jhao]]

Attachment: Adding timestamps

Mistakenly uploaded an empty patch. This one is right.

Comment 16

[Jonathan Hao (inactive) [:jhao]]

Try server: https://treeherder.mozilla.org/#/jobs?repo=try&revision=be571518a543

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/2cebf32cd8a2

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/2cebf32cd8a2

Comment 19

[Jonathan Hao (inactive) [:jhao]]

Attachment: Rebasing to 2.2 for possible uplift

Try submission:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a9de8b9968ae

Comment 20

[Jonathan Hao (inactive) [:jhao]]

Comment on attachment 8587823 [details] [diff] [review]
Rebasing to 2.2 for possible uplift

Hi Bhavana,
This uplift is for bug 1147537, which is blocking-b2g: 2.2+
Could you approve it?
Thanks.

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #):
bug 1147537

User impact if declined:
RTSP streams wouldn't autoplay, and performance would be bad

Testing completed:
Completed
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a9de8b9968ae
Although there are five builds failed, Edgar said that the reason is probably not my patch. He suggested me to push an empty patch to try server
https://treeherder.mozilla.org/#/jobs?repo=try&revision=668461393b60
and as he predicted, those five builds still failed. The reason may be that 
try server used master branch to test my 2.2 gecko code, and there's some mismatch between build systems.

Risk to taking this patch (and alternatives if risky): 
Not risky.
This patch only affects RTSP live streams.

String or UUID changes made by this patch:
None.

Comment 21

[Aaron Wu]

move this bug to 2.2+, the patch can solve bug 1149616

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/f4c5c9db2b1a