996765 - [RTSP] Support TCP-interleaved RTP transport

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RTSP, defect)

Description

[Ethan Tseng [:ethan]]

This is a follow up of bug 993924.

So far our RTSP client specifies UDP as the transport layer protocol for data delivery.
We shall also support TCP-interleaved RTP as well.

Comment 1

[Ethan Tseng [:ethan]]

Helpful information extracted from this article:
http://www.informit.com/articles/article.aspx?p=169578&seqNum=3

*** TCP-interleaved RTP ***
In certain scenarios, the best-effort, dynamic port methods of UDP-based RTP, as described previously, are not suitable. Some environments might consider the allocation of dynamic source and destination UDP ports through firewalls to be something they can live happily without. Moreover, just the nature of the Layer 1 and Layer 2 transport mechanisms underlying the data delivery might not be suited to nonguaranteed UDP traffic. In either instance, RTSP allows for the negotiation of the RTP delivery of the media data to be interleaved into the existing TCP connection.

When interleaving, the client-to-server SETUP command has the following format:

C->S  SETUP rtsp://video.foocorp.com:554/streams/example.rm RTSP/1.0
      Cseq: 3
      Transport: rtp/avp/tcp; interleaved=0-1
The changeover in the preceding example is in the transport description. First, the transport mechanisms have changed to show that the RTP delivery must be over TCP rather than UDP. Second, the addition of the interleaved option shows that the RTP data should be interleaved and use channel identifiers 0 and 1—0 will be used for the RTP data and 1 will be used for the RTCP messages. To confirm the transport setup, the server will respond with confirmation and a Session ID as before:

S->C  RTSP/1.0 200 OK
      Cseq: 3
      Session: 12345678
      Transport: rtp/ avp/tcp; interleaved=0-1
The RTP and RTCP data can now be transmitted over the existing RTSP TCP connection with the server using the 0 and 1 identifiers to represent the relevant channel.

One further delivery option for RTP and RTCP under RTSP is to wrap the delivery of all media streaming components inside traditional HTTP frame formats. This removes most barriers presented when using streaming media through firewalled environments, as even the most stringent administrator will typically allow HTTP traffic to traverse perimeter security. While HTTP and RTSP interleaved delivery of the streamed media data will make the content available to the widest possible audience, when you consider the overhead of wrapping all RTP data inside either an existing TCP stream or, worse still, inside HTTP, it is the least efficient method for delivery. To enable the streaming media client browser to cope with the different options described previously, most offer the client users the ability to configure their preferred delivery mechanism or mechanisms, and the timeout that should be imposed in failing between them. What you will see from a client perspective is that the client application will first request that the stream be delivered using RTP in UDP, and if the stream does not arrive within x seconds (as it is potentially being blocked by an intermediate firewall), it will fail back to using RTP interleaved in the existing RTSP connection.

Comment 2

[Ethan Tseng [:ethan]]

*** Note ***
The TCP-interleaving mechanism actually exists in our current code, mainly in RtspConnectionHanlder.h, which is ported from Android's MyHandler.h.

There are two reasons that result in our RTSP client does not execute this mechanism.
1. Play might be fired more than once.
This is what happened when rendering RTSP in the video app.
When the video app is launched, it fires play-pause-play events successively in a short time.
The first 'play' would trigger a 'tiou' event, which sequentially trigger a reconnect event using TCP-interleaving.
But the second 'play' immediately triggers a another 'tiou' event, and this time it causes the RtspConnectionHandler to abort and tear down the 2nd connection (which was just trying TCP-interleaving).

I found this issue would disappear when we render RTSP in the browser.
The built-in media player of the browser would not fire an additional 'play' event to RtspConnectionHandler.
So, once bug 992568 is landed, this might not be a problem any more.

2. RtspConnectionHandler should call play() itself while trying TCP-interleaving.
Even if issue 1. is resolved, there exists another issue.
Suppose there is only one 'play' event and RtspConnectionHandler enters the 2nd connection using TCP-interleaving successfully. The current code would stop after RTSP SETUP request/response. No one sends PLAY request to the server.
A naive idea is to call play() in the switch-case 'setu' if mTryTCPInterleaving is true.

Comment 3

[Ethan Tseng [:ethan]]

Attachment: WIP - Fix TCP-interleaved RTP

This patch does the following:
- Add a new message type: RtspConnectionHandler::kWhatTryTcpInterleaving.
- Send this message to notify RTSPSource that we are trying TCP interleaving.
- RTSPSource calls its performPlay() when receiving this message.

Comment 4

[Ethan Tseng [:ethan]]

*** Note ***
Q: How do I trigger the TCP-interleaving mechanism?
A: Drop incoming UDP packets on the target device.
  $ adb shell
  # iptables -i wlan0 -I INPUT -p udp --dport 15500:17000 -j DROP
  # iptables -L
  Chain INPUT (policy ACCEPT)
  target     prot opt source               destination
  DROP       udp  --  anywhere             anywhere             udp dpts:15500:17000

Comment 5

[Ethan Tseng [:ethan]]

(In reply to Ethan Tseng [:ethan] from comment #3)
> Created attachment 8412512 [details] [diff] [review]
> WIP - Fix TCP-interleaved RTP

Hi Steve,
This is a WIP patch to demonstrate my proposed solution.

It works for RTSP video but not RTSP audio.
The reason is somehow the media decoder calls pause() after play() for RTSP audio-only media.
And this issue is tracked in bug 1000195.

I would like to have your feedback whether this patch is on the right direction or not.

Comment 6

[Ethan Tseng [:ethan]]

Additional remark.
Basically my patch deals with the 2nd issue in comment 2.

The first issue in comment 2 doesn't exist with the built-in player of B2G browser.
However, it seems depending on the premise that "play() can be called only once" is not a good design contract for RtspConnectionHandler and makes the "RTP-over-TCP mechanism" not robust enough.
Maybe we need to work out something to enhance robustness.

Comment 7

[Steve Workman [:sworkman] (INACTIVE)]

Hi Ethan,

I've started looking at this. I think I understand the problem but I need to spend some more time in the code to give you better feedback. However, at a high level your approach seems like a good start. I'll spend some more time next week on this.

Comment 8

[Ethan Tseng [:ethan]]

(In reply to Steve Workman [:sworkman] from comment #7)
> Hi Ethan,
> I've started looking at this. I think I understand the problem but I need to
> spend some more time in the code to give you better feedback. However, at a
> high level your approach seems like a good start. I'll spend some more time
> next week on this.
Sure. No problem.
Thanks for you response.

Comment 9

[Steve Workman [:sworkman] (INACTIVE)]

Comment on attachment 8412512 [details] [diff] [review]
WIP - Fix TCP-interleaved RTP

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

Comments below, but after some time looking into this more, I think you're on the right track. It will just take some testing with different permutations to make sure it all works.

nit: Please don't add 'r=sworkman' to patch descriptions until I have r+d them :)

::: netwerk/protocol/rtsp/rtsp/RTSPConnectionHandler.h
@@ +697,5 @@
>                      // Notify the Rtsp Controller that we are ready to play.
>                      msg->setInt32("what", kWhatConnected);
>                      msg->post();
> +
> +                    // Notify Rtsp Controller we are trying TCP interleaving.

Looks like you're notifying RtspSource, no?

::: netwerk/protocol/rtsp/rtsp/RTSPSource.cpp
@@ +476,5 @@
> +            int64_t playTimeUs;
> +            if (!msg->findInt64("timeUs", &playTimeUs)) {
> +                playTimeUs = 0;
> +            }
> +            performPlay(playTimeUs);

I think this is fine. It looks like it ensures that there is a connection and sends RTSP 'PLAY' to start streaming data, right? Try it out and see what happens. The only concern I might have is if something had asked to suspend the stream while we were deciding to try interleaving. I'm not aware of anything that does this, but I guess it might be possible and should be looked into.

But I think you'll find that out with testing.

Comment 10

[Ethan Tseng [:ethan]]

(In reply to Steve Workman [:sworkman] from comment #9)
Hi Steve,
Thanks for your review and feedback.
I am sorry! I just found I forgot to respond to your comment (focusing on some other bugs recently).
 
> nit: Please don't add 'r=sworkman' to patch descriptions until I have r+d
> them :)
>
Oh, I just took it as a habit so that I don't need to refresh the commit message and upload the patch after r+.
I will never do that again. :)


> ::: netwerk/protocol/rtsp/rtsp/RTSPConnectionHandler.h
> > +                    // Notify Rtsp Controller we are trying TCP interleaving.
> Looks like you're notifying RtspSource, no?
> 
Yes. It should be RTSPSource. I will rectify this comment.

> ::: netwerk/protocol/rtsp/rtsp/RTSPSource.cpp
> @@ +476,5 @@
> > +            int64_t playTimeUs;
> > +            if (!msg->findInt64("timeUs", &playTimeUs)) {
> > +                playTimeUs = 0;
> > +            }
> > +            performPlay(playTimeUs);
> 
> I think this is fine. It looks like it ensures that there is a connection
> and sends RTSP 'PLAY' to start streaming data, right? Try it out and see
> what happens. The only concern I might have is if something had asked to
> suspend the stream while we were deciding to try interleaving. I'm not aware
> of anything that does this, but I guess it might be possible and should be
> looked into. 
> But I think you'll find that out with testing.
>
Actually, after some investigation with coworker of the media team, we found we cannot rely on the premise "play() is called only once".
The current design of media resource could possibly trigger "play-pause-play" events to RtspController. The pause and the 2nd play events can be treated as "internal" events, which means they are not triggered by the user.
So we have to figure out a way to deal with this problem.

In the same time, I will consider the concern you mentioned as well (if suspend is called while deciding to try interleaving).
I think I can propose a patch in a few days. Thanks again.

Comment 11

[Ethan Tseng [:ethan]]

Attachment: RTSPSource State Diagram.svg

A state machine diagram of RTSPSource object.

Comment 12

[Ethan Tseng [:ethan]]

Attachment: RTSPSource State Diagram.svg

Correct some minor errors.

Comment 13

[Ethan Tseng [:ethan]]

Attachment: Connect and Play Activity Diagram.svg

An activity diagram depicting a series of actions taken for RTSP connect and play activities.

Comment 14

[Ethan Tseng [:ethan]]

Attachment: RTSPSource State Diagram.svg

Polish the state machine diagram.

Comment 15

[Ethan Tseng [:ethan]]

Attachment: Connect and Play Activity Diagram.svg

Polish the connect and play activity diagram.

Comment 16

[Ethan Tseng [:ethan]]

Attachment: Explicitly perform play to trigger TCP-interleaved RTP

Polish comments and namings. No major change.

Comment 17

[Ethan Tseng [:ethan]]

Attachment: Explicitly perform play to leverage TCP-interleaved RTP - v2

This patch adds mNumPendingPlayTimeout to RtspConnectionHandler, to prevent from aborting a connection which is trying TCP-interleaved RTP.

Comment 18

[Benjamin Chen [:bechen]]

Comment on attachment 8420811 [details] [diff] [review]
Explicitly perform play to leverage TCP-interleaved RTP - v2

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

Please add some check() or assertsion() macro to verify the mNumPendingPlayTimeout since it is a int32 value. (overflow, underflow)

::: netwerk/protocol/rtsp/rtsp/RTSPConnectionHandler.h
@@ +1151,5 @@
> +                // events in a short period of time before the first RTP packet
> +                // is received.
> +                if (mNumPendingPlayTimeout != 1) {
> +                  return;
> +                }

Try to image a scenario, if there are 2 play commands in a sequence.
The mNumPendingPlayTimeout is 2 and never get a chance to decrease?
Do we have the mechanism  to prevent this?

Comment 19

[Ethan Tseng [:ethan]]

(In reply to Benjamin Chen [:bechen] from comment #18)
> Comment on attachment 8420811 [details] [diff] [review]
> Please add some check() or assertsion() macro to verify the
> mNumPendingPlayTimeout since it is a int32 value. (overflow, underflow)
> 
Valid concern.
mNumPendingPlayTimeout should never be a negative value, I will add an assertion, such as CHECK(mNumPendingPlayTimeout >= 0).

> ::: netwerk/protocol/rtsp/rtsp/RTSPConnectionHandler.h
> Try to image a scenario, if there are 2 play commands in a sequence.
> The mNumPendingPlayTimeout is 2 and never get a chance to decrease?
> Do we have the mechanism  to prevent this?

Yes!
From the perspective of RtspConnectionHandler, there is no mechanism to avoid adjacent play messages/events.
But the state machine of RTSPSource guarantees no duplicate play would be triggered when it is already in PLAYING state.
See:
http://dxr.mozilla.org/mozilla-central/source/netwerk/protocol/rtsp/rtsp/RTSPSource.cpp?#206

BTW, I drew the state machine diagram to help us quickly capture this idea. :)
See: attachment 8420566 [details]

Comment 20

[Ethan Tseng [:ethan]]

Attachment: Explicitly perform play to leverage TCP-interleaved RTP

This patch does two things:
1. Add a new message kWhatTryTCPInterleaving to RtspConnectionHandler.
   When RtspConnectionHandler enters the action to try to transport RTP over TCP, it sends this message to RTSPSource. RTSPSource then do performPlay() explicitly to trigger a new play event to RtspConnectionHandler.
2. Add a new private member mNumPendingPlayTimeout to RtspConnectionHandler.
  Its purpose is to prevent from aborting the connection, which is trying TCP-interleaving.

Comment 21

[Ethan Tseng [:ethan]]

Hi Steve,

I think I already clarified all the concern about this bug.
I spent some time to draw two diagrams that might help us realize the logic more clearly and quickly.
attachment 8420566 [details] - State machine of RTSPSource
attachment 8420567 [details] - Activity diagram of connect and play

First, I will answer your concern in comment 9:
"The only concern I might have is if something had asked to suspend the stream while we were deciding to try interleaving."

I guess "suspend the stream" you mentioned here can be interpreted in two meanings: "pause" or "stop".

When we are trying TCP-interleaving, it is actually in CONNECTED state (for the 2nd time), not PLAYING state. (It's more clear to follow the actions in attachment 8420567 [details] to see this).

If a "pause" is coming right now, RTSPSource would simply ignore it. Nothing happens.
(http://dxr.mozilla.org/mozilla-central/source/netwerk/protocol/rtsp/rtsp/RTSPSource.cpp?#227)

If a "stop" is coming, RTSPSource would call mHandler->disconnect() to transit to DISCONNECTED state.
Though it could be triggered either from the user side or the ConnectionHandler side, all states could be transited to DISCONNECTED state by the design. It would not be a problem.


Secondly, from the state machine and activity diagram, we could understand the essence of this bug better.
The purpose of kWhatTryTCPInterleaving is to trigger RTSPSource to transit from CONNECTED to PLAYING state because no one would send kWhatPerformPlay when we reconnect to try TCP-interleaving.

Comment 22

[Ethan Tseng [:ethan]]

Lastly, I would like to explain how I resolve the issue of "play-pause-play" events.

In RtspConnectionHandler, the 'tiou' message is only posted from the case 'play'.
Each successful 'play' (RTSP server replies 200 OK to a PLAY request) posts a 'tiou' message, which has a 10-second delay and is used to check if RTP stream data is ever received or not.

Normally this mechanism is fine.
But as I mentioned in comment 10, our Media Decoder could possibly issues "play-pause-play" events in a short period of time. It would become a nightmare if these events occur before the first RTP packet is received.
Because there two 'tiou' messages in the fly, after 10 seconds, the 1st 'tiou' triggers TCP-interleaving, but the 2nd 'tiou' would immediately posts an 'abor' message to abort the connection.

Originally, my idea is to cancel the previous 'tiou' message when 'pause' occurs.
Unfortunately I found AHandler doesn't provide a removeMessage() as Handler() does (http://developer.android.com/reference/android/os/Handler.html#removeMessages%28int%29).

That's why I introduced the counter mNumPendingPlayTimeout.
Its initial value is 0.
It is increased only in 'play'.
It could be decreased in 'pause' or 'tiou'.
We only execute 'tiou' follow-up (reconnect to try TCP-interleaving, or disconnect) when mNumPendingPlayTimeout is 1.

In the case of "play-pause-play", the 2nd 'tiou' would do nothing since mNumPendingPlayTimeout is 0.

Comment 23

[Ethan Tseng [:ethan]]

(In reply to Ethan Tseng [:ethan] from comment #22)
> Unfortunately I found AHandler doesn't provide a removeMessage() as Handler does.
*** Additional remark ***
Handler and AHandler both allow us to send and process message associated with a message queue. The former is implemented in Java; the latter is implemented in C++ in StageFright library.

I've checked the codes in AHandler.h, ALooper.h and AMessage.h. Such as:
http://androidxref.com/4.3_r2.1/xref/frameworks/av/include/media/stagefright/foundation/AHandler.h
It seems to me AHandler/ALooper/AMessage is a simpler version of Handler/Looper/Message family, and doesn't provide such abundant APIs.

Comment 24

[Steve Workman [:sworkman] (INACTIVE)]

Comment on attachment 8420852 [details] [diff] [review]
Explicitly perform play to leverage TCP-interleaved RTP

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

::: netwerk/protocol/rtsp/rtsp/RTSPConnectionHandler.h
@@ +141,5 @@
>            mReceivedFirstRTPPacket(false),
>            mSeekable(false),
>            mKeepAliveTimeoutUs(kDefaultKeepAliveTimeoutUs),
> +          mKeepAliveGeneration(0),
> +          mNumPendingPlayTimeout(0) {

nit: I first read this as being a timeout for the number of pending play commands or messages. But after your explanation I understand that it's a counter. Could you rename it to mNumPlayTimeoutsPending?

Could you also rename kStartupTimeoutUs to kPlayTimeoutUs just to make it clearer that these two are related?

@@ +724,5 @@
>              }
>              case 'pause':
>              {
>                  mPausePending = true;
> +                mNumPendingPlayTimeout--;

I'm not sure if you should do that here. I may have got this wrong, but here's how it seems to me:

There may be multiple 'tiou' messages in the queue at one time. Since we cannot get two or more consecutive 'play' messages, then the most recent 'play' is the one that we should check in 10s, right?

If that's true, then it's the last 'tiou' that is the important one. Now, you're decrementing the counter in case 'tiou' as you should and only acting if counter == 1. This seems good.

But decrementing in pause doesn't seem right. If you get 'play'->'pause'->'play' very quickly (a lot less than 10s), then the counter will be 1-1+1 == 1 when case 'tiou' is called for the first 'tiou' message. And then the second 'tiou' will be ignored.

Now, since 'play'->'pause'->'play' happens very quickly, this is probably not an issue. But is it possible for it to be slower? What happens if the underlying connection is really bad? Or if the connection is fine, but the server responds slowly - could the user cause a slow 'play'->'pause'->play'?? Perhaps that's impossible...

In any case, I think we should be timing out on the last 'tiou' message to be handled. So, I think counter++ in case 'play' and counter-- in 'tiou' only. However, it seems like we do not want to do 'tiou' if 'pause' was the most recent message, i.e. there is no 'play' to monitor and time out. So, how about this check in case 'tiou':

if (counter == 1 && mPausePending) {
...
}
ASSERT(counter > 1) {
  counter--;
}

(In fact, maybe the assertion and -- should be at the start; and then check if counter == 0)?

Does this make sense or did I miss something?

Comment 25

[Ethan Tseng [:ethan]]

Attachment: Explicitly perform play to leverage TCP-interleaved RTP

Addressed the issues in comment 24.

Comment 26

[Ethan Tseng [:ethan]]

Attachment: Explicitly perform play to leverage TCP-interleaved RTP

Just polish some code comments.

Comment 27

[Ethan Tseng [:ethan]]

(In reply to Steve Workman [:sworkman] from comment #24)
> ::: netwerk/protocol/rtsp/rtsp/RTSPConnectionHandler.h
> > +          mNumPendingPlayTimeout(0) {
> nit: I first read this as being a timeout for the number of pending play
> commands or messages. But after your explanation I understand that it's a
> counter. Could you rename it to mNumPlayTimeoutsPending?

Yes, it's a counter.
Originally I want to name it as "mPendingPlayTimeoutCount", but I found there is a member named "mNumAccessUnitsReceived". I thought to be consistent is better.
How do you think? Would it be better to use "Count" even if it's inconsistent?


> In any case, I think we should be timing out on the last 'tiou' message to
> be handled. So, I think counter++ in case 'play' and counter-- in 'tiou'
> only. However, it seems like we do not want to do 'tiou' if 'pause' was the
> most recent message, i.e. there is no 'play' to monitor and time out. So,
> how about this check in case 'tiou':
> 
> if (counter == 1 && mPausePending) {
> ...
> }
> ASSERT(counter > 1) {
>   counter--;
> }
> 
> (In fact, maybe the assertion and -- should be at the start; and then check
> if counter == 0)?
> 
> Does this make sense or did I miss something?

You are right!
We should only handle the last 'tiou' message in the queue. This is what I wanted in the first place (to remove 'tiou' message in the queue in case 'pause').
And you are right, we shouldn't decrement the counter in 'pause'. I screwed up the logic in the previous patch.

I move the assertion "CHECK(counter >= 1)" and the decrement of the counter in the beginning of case 'tiou'.
Then we only have to handle 'tiou' when counter is zero (means no pending timeout message).

I think we can't rely on mPausePending in case 'tiou'. In the case of quick "play-pause-play", the messages in the queue would be like:
'play'-'pause'-'play'-'tiou'- [other messages such 'abor', 'tear', 'disc', ...] - 'tiou'
And mPausePending is always true when program enters the case 'tiou'.
Anyway, In think correcting the manipulation of the counter is enough for the check.

Comment 28

[Steve Workman [:sworkman] (INACTIVE)]

Comment on attachment 8422458 [details] [diff] [review]
Explicitly perform play to leverage TCP-interleaved RTP

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

Thanks Ethan - looks good to me. I have a query about 'pause' followed by 'tiou', but I assume you have that covered. So, r=me - if you need to add something else, I can review that again.

::: netwerk/protocol/rtsp/rtsp/RTSPConnectionHandler.h
@@ +141,5 @@
>            mReceivedFirstRTPPacket(false),
>            mSeekable(false),
>            mKeepAliveTimeoutUs(kDefaultKeepAliveTimeoutUs),
> +          mKeepAliveGeneration(0),
> +          mNumPlayTimeoutPending(0) {

You asked if I think you should add "Count" here, but I think this is fine. "Num" at the start and "Pending" at the end works great.

But just add an 's' to Timeout --> mNumPlayTimeoutsPending.

Man, sometimes naming these things is a pain :)

@@ +1155,5 @@
> +                // TCP.
> +                if (mNumPlayTimeoutPending > 0) {
> +                  // Do nothing. We only handle the last 'tiou' message.
> +                  return;
> +                }

Cool. Decrement first, then only handle the timeout if we're at 0, resulting in only the most recent 'tiou' being handled.

So, in effect, this means that the 'play' timer is reset every time we issue a 'play'. Sounds good to me.

Now, you said we don't need to worry about 'pause' - I assume that the 'tiou' handling code here deals with that? So, we're not going to get a connection retry if 'pause' was one of the last messages to be fired?

Comment 29

[Ethan Tseng [:ethan]]

Attachment: Explicitly perform play to leverage TCP-interleaved RTP

Fixed a nit (variable naming) in comment 28.

Comment 30

[Ethan Tseng [:ethan]]

(In reply to Steve Workman [:sworkman] from comment #28)
> -----------------------------------------------------------------
> Thanks Ethan - looks good to me. I have a query about 'pause' followed by
> 'tiou', but I assume you have that covered. So, r=me - if you need to add
> something else, I can review that again.
> 
Thanks a lot for taking the time to review this.
So far I think nothing else is needed for RTP-over-TCP. :)

> But just add an 's' to Timeout --> mNumPlayTimeoutsPending. 
> Man, sometimes naming these things is a pain :)
> 
Haha, that's true!
Naming is a pain but also a key of clean code.
Thanks for you kindly help to provide so many wonderful suggestions. Really appreciated. :)


> So, in effect, this means that the 'play' timer is reset every time we issue
> a 'play'. Sounds good to me.
> 
> Now, you said we don't need to worry about 'pause' - I assume that the
> 'tiou' handling code here deals with that? So, we're not going to get a
> connection retry if 'pause' was one of the last messages to be fired?

I am not sure I totally understand this paragraph.
But I do intend to cancel to reconnect to try TCP-interleaving if 'pause'-'play' happens before the 'tiout' triggered by the first 'play'.

Comment 31

[Ethan Tseng [:ethan]]

attachment 8422974 [details] [diff] [review] is r+ and ready to land.

The result of Try server:
Syntax: try: -b o -p all -u crashtest-1,crashtest-2,crashtest-3 -t none
Result: https://tbpl.mozilla.org/?tree=Try&rev=716e9873e63e

Comment 32

[Steve Workman [:sworkman] (INACTIVE)]

(In reply to Ethan Tseng [:ethan] from comment #30)
> (In reply to Steve Workman [:sworkman] from comment #28)
> > So, in effect, this means that the 'play' timer is reset every time we issue
> > a 'play'. Sounds good to me.
> > 
> > Now, you said we don't need to worry about 'pause' - I assume that the
> > 'tiou' handling code here deals with that? So, we're not going to get a
> > connection retry if 'pause' was one of the last messages to be fired?
> 
> I am not sure I totally understand this paragraph.
> But I do intend to cancel to reconnect to try TCP-interleaving if
> 'pause'-'play' happens before the 'tiout' triggered by the first 'play'.

Sorry about that. My question is what happens when 'tiou' is received by RTSPConnection after we have disconnected. Since there is no way to cancel messages, it seems like this will still be delivered. I assume the code already handles this fine.

I was thinking about it specifically in relation to multiple 'play'-'pause' combinations, resulting in multiple 'tiou' messages ... with your patch we only process the final 'tiou' ... that is still ok if we have disconnected, right?

I don't think this is a big issue; it's more to do with improving my understanding of the code :)

Comment 33

[Ethan Tseng [:ethan]]

(In reply to Steve Workman [:sworkman] from comment #32) 
> Sorry about that. My question is what happens when 'tiou' is received by
> RTSPConnection after we have disconnected. Since there is no way to cancel
> messages, it seems like this will still be delivered. I assume the code
> already handles this fine.
> 
> I was thinking about it specifically in relation to multiple 'play'-'pause'
> combinations, resulting in multiple 'tiou' messages ... with your patch we
> only process the final 'tiou' ... that is still ok if we have disconnected,
> right?
> 
> I don't think this is a big issue; it's more to do with improving my
> understanding of the code :)

No problem, I'd love to explain this.

Once disconnected, no matter it is trigger by the user or any network error, it would eventually call RTSPSource::onDisconnected(), which reports disconnected to upstream object (RtspController) and releases it self.
This function calls unregisterHandler().
http://dxr.mozilla.org/mozilla-central/source/netwerk/protocol/rtsp/rtsp/RTSPSource.cpp#583
Once this line is executed, RtspConnectionHandler (mHandler) is unregistered from RTSPSource's mLooper and will never receive any message from now on.

In conclusion, RtspConnectionHandler will never receive 'tiou' after disconnected.
Hope I answer your question. :)

Comment 34

[Ethan Tseng [:ethan]]

Additionally, after RTSPSource::onDisconnected() reports disconnected to upstream, the RTSPSource instance will be destructed soon. This means the event queue (member of ALooper) will be destructed along with RTSPSource. So, any remaining messages (including 'tiou') will no longer exist after disconnected.

Comment 35

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/b2g-inbound/rev/318e7b05b8cd

Comment 36

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/318e7b05b8cd

Comment 37

[Ming]

[Blocking Requested - why for this release]:
v1.4 need this patch .
Plz help

Comment 38

[Ryan VanderMeulen [:RyanVM]]

I have nothing to do with making those decisions.

Comment 39

[Wayne Chang [:wchang]]

Not adding new features on 1.4.