1133461 - [RTSP] Remove the extra PLAY request for seek operation

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RTSP, defect)

Description

[Ethan Tseng [:ethan]]

RTSP spec does not define "SEEK" method.
When we perform a seek operation in an RTSP clip, actually it sends PAUSE and
PLAY requests to the server.

However, our current implementation sends an unnecessary PLAY request to the
server before the PAUSE.
It is because MediaOmxReader::Seek() calls EnsureActive().

In bug 1117486, we found the additional PLAY request has the potential to crash
the whole system when RTSP connection is broken.
(See https://bugzilla.mozilla.org/show_bug.cgi?id=1117486#c128 for detail.)

Even if the patch in bug 1117486 avoids the crash, it could still make the server
send some RTP stream data that will immediately be dropped by the client.
(Because the client is in seeking state.)
e.g.
02-14 13:46:57.677 I/RTSP    (  207): RtspConnectionHandler::play called
02-14 13:46:57.877 I/RTSP    (  207): RtspConnectionHandler::seek called
02-14 13:46:58.097 I/RTSP    (  207): we're seeking, dropping stale packet.
02-14 13:46:58.097 I/RTSP    (  207): we're seeking, dropping stale packet.
...


In summary, the extra PLAY request could either disarrange the error handling of
RTSP stack or impair its performance. More importantly, it should be here.
It is sent simply by reckless design. We should remove it.

Comment 1

[Ethan Tseng [:ethan]]

(In reply to Ethan Tseng [:ethan] from comment #0)
> RTSP stack or impair its performance. More importantly, it should be here.
Typo. "It (the extra PLAY request) should NOT be here."

Comment 2

[Ethan Tseng [:ethan]]

Attachment: Seek after EOS.svg

Sequence diagram showing the messages passed between RtspOmxReader,
RtspMediaResource, RtspController, RTSPSource and RtspConnectionHandler
when seeking after end-of-stream.

Comment 3

[Ethan Tseng [:ethan]]

*** Root Cause Analysis ***
Open the sequence diagram in attachment 8569735 [details].
This sequence diagram shows what happens in a seek operation. It ignores the parts that are irrelevant to this bug and only shows the objects/messages we care about here.

When we re-play an RTSP clip after playback ended, actually the underlying media decoder is performing a seek operation. And we found two problems in this seek operation.

The first problem is message (5). When RTSPSource::seek() is invoked, it posts the performSeek command with a 200 ms delay, which we think was intended to avoid performing multiple seeks in a short duration.
(https://android.googlesource.com/platform/frameworks/av/+/ee736e9e74c5368db8d63214513c85cb74bb0183)

So RTSPSource always postpones the seek operation for 200 ms. Originally this should not be an issue. Unfortunately because of the mistake in the second problem, it could bring a nasty bug.

The second problem is message (8). MediaOmxReader::Seek() calls EnsureActive, which is the version overridden by RtspOmxReader, and it always calls RtspController::Play(). Since the seek operation is delayed, this play operation will be performed earlier than the seek. In other words, the order of the seek and play operations are reversed in this scenario.

In the case of bug 1117486, RTSP connection is broken for some reason, so the play (message 12) and the seek (message 13) requests both failed. The nasty bug appears when the error handling flows of play and seek requests are interleaved, which becomes the root cause of bug 1117486.
(https://bugzilla.mozilla.org/show_bug.cgi?id=1117486#c128)


*** Solution ***
The two problems described above should both be resolved.

First, we don't have to delay the seek operation in RTSPSource. MediaDecoderStateMachine already has some mechanism to avoid performing too-many seeks in a short period (such as dragging the seek bar), so delaying the seek in somewhere else doesn't benefit a lot.

Second, RtspOmxReader::EnsureActive() does not have to call RtspController::Play() if the EnsureActive is triggered by a seek operation.  The purpose of EnsureActive is to make certain the underlying hardware is not idle. But this is meaningless to RTSP streaming.

Comment 4

[Ethan Tseng [:ethan]]

Attachment: bug-1133461-fix.patch

Benjamin,
This patch implements the solution proposed in comment 3.
Could you help to review it?

Comment 5

[Benjamin Chen [:bechen]]

Comment on attachment 8572541 [details] [diff] [review]
bug-1133461-fix.patch

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

::: netwerk/protocol/rtsp/rtsp/RTSPSource.cpp
@@ +217,5 @@
>  status_t RTSPSource::seekTo(int64_t seekTimeUs) {
>      sp<AMessage> msg = new AMessage(kWhatPerformSeek, mReflector->id());
>      msg->setInt32("generation", ++mSeekGeneration);
>      msg->setInt64("timeUs", seekTimeUs);
> +    msg->post();

Please leave some comments for the change.

Comment 6

[Ethan Tseng [:ethan]]

Attachment: bug-1133461-fix.patch

Add code comments according to reviewer's suggestion.

Benjamin, could you please help to see if the description is correct?

Comment 7

[Ethan Tseng [:ethan]]

The result of Treeherder:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7359088afab4

Comment 8

[Ethan Tseng [:ethan]]

While resolving this bug, we realize the current RtspOmxReader::EnsureActive() is not proper.
Thus, a follow-up is filed.
Bug 1141412 - [RTSP] Tweak EnsureActive to make it more suitable for RTSP streaming

Comment 9

[Carsten Book [:Tomcat]]

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

Comment 10

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

https://hg.mozilla.org/mozilla-central/rev/c82c6d730a3b