1021006 - [RTSP][2.0] Browser pops up a network warning message in the end of video RTSP streaming

Status: VERIFIED FIXED

(Firefox OS Graveyard :: RTSP, defect)

Description

[William Hsu [:whsu]]

Attachment: WP_20140605_018.mp4

* Description:
  This problem happened on "video" RTSP streaming.
  After built-in media player stops the video RTSP streaming, you will see the network warning message pops up.
  Attach the demo video and log. FYI.
  - Video: WP_20140605_018.mp4
  - Log: RTSP_Network_Message_20140605

* Reproduction steps:
  1. Launch the following web page via browser
     - http://goo.gl/lE2eE3
  2. Tap the "Video test page"
  3. Move indicator of seekbar to the last 10 seconds
  4. Waiting for the end

* Expected result:
  Indicator returns to 00:00 and you can replay video RTSP streaming

* Actual result:
  A network warning message pops up

* Build information: (V2.0 - Flame)
  - Gaia      a38a6a5c6fabc97dd16d5360632b5ac5c7e06241
  - Gecko     https://hg.mozilla.org/mozilla-central/rev/951e3a671279
  - BuildID   20140604160202
  - Version   32.0a1

Comment 1

[William Hsu [:whsu]]

Attachment: RTSP_Network_Message_20140605

Comment 2

[William Hsu [:whsu]]

Attachment: 2014-06-05-22-56-43.png

Comment 3

[Preeti Raghunath(:Preeti)]

Misleading UX.

Comment 4

[howie [:howie]]

Hi Ethan, please help on this, thanks.

Comment 5

[Ethan Tseng [:ethan]]

(In reply to howie [:howie] from comment #4)
> Hi Ethan, please help on this, thanks.

Sure. I will look into this issue.

Comment 6

[Benjamin Chen [:bechen]]

Attachment: bug-1021006-rtsp-statemachine.patch

Hi Ethan:
As discussed, the wip patch that can listen the state change of MediaDecoderStateMachine.

Comment 7

[Ethan Tseng [:ethan]]

Howie,
The root cause was identified. It's our duty.
I think we can fix it in several days.

Comment 8

[Ethan Tseng [:ethan]]

(In reply to Benjamin Chen [:bechen] from comment #6)
> Created attachment 8441263 [details] [diff] [review]
> bug-1021006-rtsp-statemachine.patch
> Hi Ethan:
> As discussed, the wip patch that can listen the state change of
> MediaDecoderStateMachine.

Thanks man!
I will follow up based on your patch.

Comment 9

[Ethan Tseng [:ethan]]

Attachment: Add playbackEnded to RtspController

Add playbackEnded() to nsIStreamingProtocolController and RTSPSource.cpp.
The purpose is to ensure RTSPSource transit from PLAYING to CONNECTED state at the right moment.

Comment 10

[Ethan Tseng [:ethan]]

Comment on attachment 8442591 [details] [diff] [review]
Add playbackEnded to RtspController

Hello Benjamin,

This patch applies the solution we discussed offline. Could you please review it?
Any feedback is welcome.

Comment 11

[Ethan Tseng [:ethan]]

Attachment: RTSPSource State Diagram.svg

The state machine diagram of RTSPSource.

Comment 12

[Ethan Tseng [:ethan]]

Comment on attachment 8442591 [details] [diff] [review]
Add playbackEnded to RtspController

Hello Steve,

I will require your review and would like to have your feedback first.

=== About End-of-Stream ===
As you know, the specs of RTSP/RTP don't define a specific "end-of-stream" notification mechanism. The client-side has to detect end-of-stream itself, which is implementation-dependent.

In bug 951278, we added a 2-second timer to detect an end of stream event. When it timeouts, we produce an artificial end-of-stream frame per track and send the frame to the media decoder.
For reminder:
https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=951278&attachment=8426165


=== What is the problem? ===
The essential problem of the patch of bug 951278 is about state transition.

The current design of the media decoder is to perform the play operation constantly when the decoder is in the state PLAYING_STATE_PLAYING. The call path is: RtspOmxReader::EnsureActive() -> RtspController::Play() -> RTSPSource::play() -> RTSPSource::performPlay().
The last function would not actually send an RTSP PLAY request because it is already in PLAYING state.

However, RTSPSource::onTrackEndOfStream() changes the state from PLAYING to CONNECTED immediately, which creates a "hole" to send out an improper PLAY request to the RTSP server. In our observation, there are at least two different scenarios resulted from this behavior.
1. The server replies 200 OK to the PLAY request.
   Since the media decoder is already in ended state, we won't see anything being render on the screen. But the streaming data is actually being transmitted. This could be the root cause of bug 1017444.
2. The server replies 457 error (invalid range) to the PLAY request.
   RTSP session will be disconnected and an error message will be displayed, which is the scenario of this bug.


=== Solution ===
We think the essence of this problem is, RTSPSource cannot change its state from PLAYING to CONNECTED on itself since it doesn't know when the playing state is ended.
So we introduce a new function "PlaybackEnded()", in order to let RtspOmxReader notify RTSPSource when the decoder is in the state PLAYING_STATE_ENDED. RTSPSource performs "PLAYING->CONNECTED" state transition according to this notification.
In this way, we can guarantee no improper play would be performed.

The new transition is marked in red color in the state machine: attachment 8442676 [details].

Comment 13

[Benjamin Chen [:bechen]]

Comment on attachment 8442591 [details] [diff] [review]
Add playbackEnded to RtspController

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

::: netwerk/protocol/rtsp/controller/RtspController.cpp
@@ +376,5 @@
> +  if (!mRtspSource.get()) {
> +    MOZ_ASSERT(mRtspSource.get(), "mRtspSource should not be null!");
> +    return NS_ERROR_NOT_INITIALIZED;
> +  }
> +

How about
MOZ_ASSERT(mRtspSource.get(), "mRtspSource should not be null!");
NS_ENSURE_TRUE(mRtspSource.get(), NS_ERROR_NOT_INITIALIZED)

::: netwerk/protocol/rtsp/controller/RtspControllerParent.cpp
@@ +170,5 @@
> +bool
> +RtspControllerParent::RecvPlaybackEnded()
> +{
> +  LOG(("RtspControllerParent::RecvPlaybackEnded()"));
> +  NS_ENSURE_TRUE(mController, true);

Should we return false here?

Comment 14

[Ethan Tseng [:ethan]]

(In reply to Benjamin Chen [:bechen] from comment #13)
> Review of attachment 8442591 [details] [diff] [review]:
> -----------------------------------------------------------------
> How about
> MOZ_ASSERT(mRtspSource.get(), "mRtspSource should not be null!");
> NS_ENSURE_TRUE(mRtspSource.get(), NS_ERROR_NOT_INITIALIZED)
> 
It's fine to me.But I want the code to be consistent.
If we change this, we should change the same snippets throughout this file.

> ::: netwerk/protocol/rtsp/controller/RtspControllerParent.cpp
> @@ +170,5 @@
> > +bool
> > +RtspControllerParent::RecvPlaybackEnded()
> > +{
> > +  LOG(("RtspControllerParent::RecvPlaybackEnded()"));
> > +  NS_ENSURE_TRUE(mController, true);
> 
> Should we return false here?

Maybe not. Refer to:
https://developer.mozilla.org/en-US/docs/IPDL/Tutorial#Shutdown_and_Error_Handling
"The C++ methods which implement IPDL messages return bool: true for success, and false for catastrophic failure. Message implementations should return false from a message implementation if the data is corrupted or otherwise malformed. .... Do not return false from message handlers for "normal" error conditions such as inability to load a network request! Normal errors should be signaled with a message or return value."

However, we do lack of a normal error handling right now. It could be an improvement issue.

Comment 15

[Ethan Tseng [:ethan]]

Comment on attachment 8442591 [details] [diff] [review]
Add playbackEnded to RtspController

Hi Steve,

I already tested my patch and it works well.
I would like to proceed the review process directly.
You can still refer to my explanation in comment 12.

Comment 16

[Steve Workman [:sworkman] (INACTIVE)]

Comment on attachment 8442591 [details] [diff] [review]
Add playbackEnded to RtspController

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

If I've understood this right, this patch is about RtspSource not transitioning from PLAYING to CONNECTED on its own; instead, it must wait for RtspOmxDecoder to notify it (indirectly) that playback has ended. I think this much is obvious from your description and the patch itself. However, I'm not 100% clear about the 'hole'. Can you describe the series of events that would happen during the hole, please? Specifically, how does the kWhatPerformPlay/performPlay get sent/called if playback has already ended? The rest of your description of the problem is good; I'm just not clear on this.

::: content/media/omx/RtspOmxDecoder.cpp
@@ +25,5 @@
>  
> +void RtspOmxDecoder::ApplyStateToStateMachine(PlayState aState)
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +  // Notify RTSP controller if the play state is ended.

nit: newline after MOZ_ASSERT.

@@ +37,5 @@
> +        controller->PlaybackEnded();
> +      }
> +    }
> +  }
> +  MediaDecoder::ApplyStateToStateMachine(aState);

Please verify that controller->PlaybackEnded() should be called BEFORE the MediaDecoder state machine is updated. Is AFTER better? Maybe it's not important if it's before or after ;)

::: content/media/omx/RtspOmxDecoder.h
@@ +31,5 @@
>    }
>  
>    virtual MediaDecoder* Clone() MOZ_OVERRIDE;
>    virtual MediaDecoderStateMachine* CreateStateMachine() MOZ_OVERRIDE;
> +  virtual void ApplyStateToStateMachine(PlayState aState) MOZ_OVERRIDE;

'virtual' isn't actually needed if the base class function is already declared virtual.
Also, we can probably mark these functions with MOZ_FINAL too, because I don't see any other class extending RtspOmxDecoder. But probably this is a cleanup in a separate bug.

::: netwerk/base/public/nsIStreamingProtocolController.idl
@@ +175,5 @@
>  
> +    /*
> +     * Tell the streaming server the playback is in ended state.
> +     */
> +    void playbackEnded();

I don't think the streaming server is told anything here, right? This is just about notifying RtspSource that it should move from PLAYING to CONNECTED states, no?

::: netwerk/protocol/rtsp/controller/RtspController.cpp
@@ +376,5 @@
> +  if (!mRtspSource.get()) {
> +    MOZ_ASSERT(mRtspSource.get(), "mRtspSource should not be null!");
> +    return NS_ERROR_NOT_INITIALIZED;
> +  }
> +

Please don't use NS_ENSURE_TRUE. It's nicer to look at, but there is a note and explanation in http://dxr.mozilla.org/mozilla-central/source/xpcom/glue/nsDebug.h#281 to say that it's deprecated.

::: netwerk/protocol/rtsp/rtsp/RTSPSource.cpp
@@ +253,5 @@
>  // TODO, Bug 895753.
>  }
>  
> +void RTSPSource::performPlaybackEnded() {
> +    // Transit from PLAYING to CONNECTED state so that we are ready to perform a

"Transition from ..."

::: netwerk/protocol/rtsp/rtsp/RTSPSource.h
@@ +78,5 @@
> +        kWhatPerformPlay          = 'play',
> +        kWhatPerformPause         = 'paus',
> +        kWhatPerformResume        = 'resu',
> +        kWhatPerformSuspend       = 'susp',
> +        kWhatPerformPlaybackEnded = 'pend',

'pend' looks like it's short for pending. I think 'ende' is better here.

Is there some whitespace change in this enum? It looks like all the members have been changed in the diff.

Comment 17

[Benjamin Chen [:bechen]]

Comment on attachment 8442591 [details] [diff] [review]
Add playbackEnded to RtspController

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

::: content/media/omx/RtspOmxDecoder.cpp
@@ +37,5 @@
> +        controller->PlaybackEnded();
> +      }
> +    }
> +  }
> +  MediaDecoder::ApplyStateToStateMachine(aState);

It's not important here, but it seems more readable  if we call MediaDecoder::ApplyStateToStateMachine(aState) first.

Comment 18

[Ethan Tseng [:ethan]]

(In reply to Benjamin Chen [:bechen] from comment #17)
> It's not important here, but it seems more readable  if we call
> MediaDecoder::ApplyStateToStateMachine(aState) first.

Thanks!
Then I'll change the location of this invocation.

Comment 19

[Ethan Tseng [:ethan]]

(In reply to Steve Workman [:sworkman] from comment #16)
> Review of attachment 8442591 [details] [diff] [review]:
> -----------------------------------------------------------------
> However, I'm not 100% clear about the 'hole'. Can you describe the series of
> events that would happen during the hole, please? Specifically, how does the
> kWhatPerformPlay/performPlay get sent/called if playback has already ended?
> The rest of your description of the problem is good; I'm just not clear on
> this.

I'll try to explain what I know.

Previously, we can treat the "play" operations in HTMLMediaElement, MediaDecoder and RtspController equally.
Which means these plays have an almost one-to-one relationship.
But there was a significant change to media decoding in bug 973408, which aimed to enable multiple decoders run concurrently.
This change resulted in that the play of HTMLMediaElement doesn't commensurate with the play of MediaDecoder any more.

The point here is about "MediaOmxReader::EnsureActive()".
It is called inside DecodeVideoFrame and DecodeAudioData, and it calls mOmxDecoder->Play().
When MediaDecoderStateMachine is going to decode an audio/video task, it calls EnsureActive() to ensure the hardware decoder is in an active state.
So EnsureActive() is called for each decoding task, and mOmxDecoder will determine to change its internal state or not.

RtspOmxReader inherits MediaOmxReader to reuse its audio/video decoding logic.
That's why RtspOmxReader::EnsureActive() will call RtspController's play() multiple times within a single second.
Because there is a latency between "RTSPSource receives an end-of-stream event" and "MediaDecoderStateMachine reaches the PLAYING_STATE_ENDED state", if we transit RTSPSource to CONNECTED state too early, the frequent play() triggered from EnsureActive() would cause RTSPSource send out an improper PLAY request.
Moreover, this latency is amplified in the case of video streaming because there are two end-of-stream events on audio and video tracks.

This is AFAIK. If anything is not clear enough, please let me know. :)

Comment 20

[Ethan Tseng [:ethan]]

Attachment: Add playbackEnded to RtspController

Addressed the issues in the review comment.

Comment 21

[Ethan Tseng [:ethan]]

(In reply to Steve Workman [:sworkman] from comment #16)
> Comment on attachment 8442591 [details] [diff] [review]
> nit: newline after MOZ_ASSERT.
> Please verify that controller->PlaybackEnded() should be called BEFORE the
> MediaDecoder state machine is updated. Is AFTER better? Maybe it's not
> important if it's before or after ;)

Both these nits were fixed.

> ::: content/media/omx/RtspOmxDecoder.h
> >    virtual MediaDecoder* Clone() MOZ_OVERRIDE;
> >    virtual MediaDecoderStateMachine* CreateStateMachine() MOZ_OVERRIDE;
> > +  virtual void ApplyStateToStateMachine(PlayState aState) MOZ_OVERRIDE;
> 
> 'virtual' isn't actually needed if the base class function is already
> declared virtual.
> Also, we can probably mark these functions with MOZ_FINAL too, because I
> don't see any other class extending RtspOmxDecoder. But probably this is a
> cleanup in a separate bug.

I added MOZ_FINAL for these three functions.
However, I think we should "virtual" here to make it more clear.
Besides, according to https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style
"Method declarations that override virtual methods from a base class should be annotated with both "virtual" and "MOZ_OVERRIDE"."


> ::: netwerk/base/public/nsIStreamingProtocolController.idl
> > +    /*
> > +     * Tell the streaming server the playback is in ended state.
> > +     */
> > +    void playbackEnded();
> I don't think the streaming server is told anything here, right? This is
> just about notifying RtspSource that it should move from PLAYING to
> CONNECTED states, no?

You are right. The comment was corrected.


> ::: netwerk/protocol/rtsp/rtsp/RTSPSource.h
> @@ +78,5 @@
> > +        kWhatPerformPlay          = 'play',
> > +        kWhatPerformPause         = 'paus',
> > +        kWhatPerformResume        = 'resu',
> > +        kWhatPerformSuspend       = 'susp',
> > +        kWhatPerformPlaybackEnded = 'pend',
> 
> 'pend' looks like it's short for pending. I think 'ende' is better here.

Agreed. Corrected!
 
> Is there some whitespace change in this enum? It looks like all the members
> have been changed in the diff.

Yes. Whitespaces were added just for alignment.

Comment 22

[Ethan Tseng [:ethan]]

(In reply to Ethan Tseng [:ethan] from comment #21)
> However, I think we should "virtual" here to make it more clear.
A typo.
However, I think we should keep "virtual" here to make it more clear.

Comment 23

[Steve Workman [:sworkman] (INACTIVE)]

Comment on attachment 8445775 [details] [diff] [review]
Add playbackEnded to RtspController

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

Thanks for the great explanation - that makes much more sense to me now. r=me.

::: content/media/omx/RtspOmxDecoder.cpp
@@ +27,5 @@
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +
> +  MediaDecoder::ApplyStateToStateMachine(aState);
> +  // Notify RTSP controller if the play state is ended.

nit (really minor one!): Add a new line after the parent class's function is called. It's just a little clearer to have the function visually separated like this.

::: content/media/omx/RtspOmxDecoder.h
@@ +32,5 @@
>  
> +  virtual MediaDecoder* Clone() MOZ_OVERRIDE MOZ_FINAL;
> +  virtual MediaDecoderStateMachine* CreateStateMachine() MOZ_OVERRIDE MOZ_FINAL;
> +  virtual void ApplyStateToStateMachine(PlayState aState)
> +                                        MOZ_OVERRIDE MOZ_FINAL;

OK re virtual - I wasn't aware of the style guide advice on that actually ;)

::: netwerk/base/public/nsIStreamingProtocolController.idl
@@ +173,5 @@
>       */
>      void stop();
>  
> +    /*
> +     * Notify the streaming controller to transit from PLAYING to CONNECTED

"transition" - transit is more often to do with carrying something, like "public transit" (noun); transition is about change of state. But you have the right idea :)

Comment 24

[Ethan Tseng [:ethan]]

(In reply to Steve Workman [:sworkman] from comment #23)
> Review of attachment 8445775 [details] [diff] [review]:
> -----------------------------------------------------------------
> Thanks for the great explanation - that makes much more sense to me now.
> r=me.
> 
Thank you, Steve!

> ::: content/media/omx/RtspOmxDecoder.cpp
> nit (really minor one!): Add a new line after the parent class's function is
> called. It's just a little clearer to have the function visually separated
> like this.
> 
Actually, personally I like to add new lines to separate code paragraphs to make them visually clearer.
But I found many codes in Mozilla are more compact without many new lines and I tried to adapt to that style.
It appears that it's a personal preference of style. Anyway, I agree with your advice.

> ::: netwerk/base/public/nsIStreamingProtocolController.idl
> "transition" - transit is more often to do with carrying something, like
> "public transit" (noun); transition is about change of state. But you have
> the right idea :)
> 
Thanks for your explanation.
As a non-native English speaker, I am happy to learn adequate English in any way. :)

Comment 25

[Ethan Tseng [:ethan]]

Attachment: Add playbackEnded to nsIStreamingProtocolController/RtspController

Fixed nits stated in comment 23.

Comment 26

[Ethan Tseng [:ethan]]

The result of Try server:
https://tbpl.mozilla.org/?tree=Try&rev=d363e108fcfd

Comment 27

[Carsten Book [:Tomcat]]

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

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4dc4793f8f71

Comment 29

[Ethan Tseng [:ethan]]

Hi William,

We resolved this bug. The improper error message won't appear when the streaming ends.
However, if you try to re-play the streaming again, the error message could still possibly appear with certain RTSP servers (e.g. Youtube).
I filed bug 1031170 to track this issue.
It could be a server-side issue, not ours.

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/ae91134619c5

Comment 31

[William Hsu [:whsu]]

Thanks Ethan! Verified this patch.
The network warning message doesn't show on the end of streaming
I would like to use bug 1031170 to trace remaining work.
Have a nice weekend!

Attach the screenshot. (2014-07-04-20-33-53.png)


* Build information:
 - Gaia      b597b86274ab109d7ef530bc0c4b6adccddae4e4
 - Gecko     https://hg.mozilla.org/mozilla-central/rev/e8df6826a571
 - BuildID   20140704040205
 - Version   33.0a1

Comment 32

[William Hsu [:whsu]]

Attachment: 2014-07-04-20-33-53.png

Comment 33

[Ethan Tseng [:ethan]]

(In reply to William Hsu [:whsu] from comment #31)
> Thanks Ethan! Verified this patch.
> The network warning message doesn't show on the end of streaming
> I would like to use bug 1031170 to trace remaining work.
> Have a nice weekend!

You're welcome.
Thanks for your verification! :)