[RTSP] Playback jumps to the end-of-stream after playing 34 seconds

RESOLVED FIXED in Firefox 40, Firefox OS v2.0M

Status

Firefox OS
RIL
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: ethan, Assigned: jhao)

Tracking

unspecified
2.2 S11 (1may)
ARM
Gonk (Firefox OS)

Firefox Tracking Flags

(blocking-b2g:2.0M+, firefox40 fixed, b2g-v2.0M fixed)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

3 years ago
This bug is found while debugging bug 1146251.

Reproduction Steps:
1. Open this web page in browser app.
   http://60.12.220.48/test/test.html
2. Click the clip "long_1.mp4".
3. Wait for 34~35 seconds, the playback suddenly jumps to the end and stops.

Expected Result:
The player can play the video as normal as other clips.

Additional Comment:
1. Codec Information
   Video Codec: H264 - MPEG-4 AVC (part 10) (h264)
   Video Resolution: 512x288
   Video Frame Rate: 15
   Video Decoded format: Planar 4:2:0 YUV
   Audio Codec: MPEG AAC Audio (mp4a)
   Audio Sample rate: 44100 Hz
   Audio AAC extension: SBR

2. VLC player desktop and Android phone both can play this RTSP clip normally.
   Benjamin guesses our RTSP stack encounters some kind of decode error
   while dealing with this clip.

3. VLC player shows there are totally four tracks in this RTSP clip.
   Two video tracks and two audio tracks.
(Reporter)

Comment 1

3 years ago
Jonathan, please help to investigate this bug.
Assignee: nobody → jhao
Status: NEW → ASSIGNED
(Reporter)

Comment 2

3 years ago
Created attachment 8588987 [details]
Codec information
(Assignee)

Comment 3

3 years ago
Currently, we check whether there are any new access units received every 2 seconds.
Maybe it's the server's problem; this stream lags for a couple of seconds at the 34 second point.
This lagging can also be observed on Android or VLC desktop.
No access units are received in these seconds and cause the player to jump to end-of-stream.

That check was originally done every 10 seconds, and was used to check network connection.
When it was used as end-of-stream criteria, the timeout was reduced to 2 seconds.
Benjamin, Ethan and I think we should separate this check from the end-of-stream criteria.

We can write another timer with 2-second timeout, and only initiates it when the stream is closed to the end of duration.
(Assignee)

Comment 4

3 years ago
If we look at the timestamps of received access units, we would find there's always about 1.5 seconds missing at around 34 seconds.
(Assignee)

Comment 5

3 years ago
Created attachment 8591582 [details] [diff] [review]
Separate timer of access unit timeout and end-of-stream detection.

This patch add a timer for end-of-stream detection, and increase original timer to 10 seconds to avoid false timeout.

I cannot test some use cases because of bug 1145052. I'll do more tests after that bug is fixed, and then I'll ask for Ethan's review.
(Reporter)

Comment 6

3 years ago
Comment on attachment 8591582 [details] [diff] [review]
Separate timer of access unit timeout and end-of-stream detection.

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

It's a good thing to separate these two timers.
Thanks for your patch! I'll review it thoroughly when you're ready. :)

::: netwerk/protocol/rtsp/rtsp/RTSPConnectionHandler.h
@@ +51,5 @@
> +static int64_t kAccessUnitTimeoutUs = 10000000ll;
> +
> +// If no access units are received within the final 2 secs of duration,
> +// assume that the rtp stream has ended and signal end of stream.
> +static int64_t kEndOfStreamUs = 2000000ll;

s/kEndOfStreamUs/kEndOfStreamTimeoutUs/
(Assignee)

Comment 7

3 years ago
Created attachment 8595836 [details] [diff] [review]
Separate timer of access unit timeout and end-of-stream detection.

Hi Ethan, this patch is ready.
I've tested "seeking to near the end" based on bug 1145052's patch and there are no problems.
Attachment #8591582 - Attachment is obsolete: true
Attachment #8595836 - Flags: review?(ettseng)
(Reporter)

Comment 8

3 years ago
Comment on attachment 8595836 [details] [diff] [review]
Separate timer of access unit timeout and end-of-stream detection.

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

The patch looks good overall.
But I have a question and a minor suggestion. Feedback+.

::: netwerk/protocol/rtsp/rtsp/RTSPConnectionHandler.h
@@ +965,5 @@
> +                    // This is an outdated message. Ignore.
> +                    break;
> +                }
> +
> +                if (mNumAccessUnitsReceived == 0) {

Is there a reason to remove this code fragment?

/* missing code fragment */
if (gIOService->IsOffline()) {	
    sp<AMessage> reply = new AMessage(kWhatDisconnected, id());	
    reply->setInt32("result", ERROR_CONNECTION_LOST);	
    mConn->disconnect(reply);	
    break;	
}
/* end of missing code */

@@ +1082,5 @@
>                  }
>  
>                  onAccessUnitComplete(trackIndex, accessUnit);
> +
> +                // put this code here because now accessUnit has timestamp

nit: Capitalize the first letter.

@@ +1087,5 @@
> +                track->mNumAccessUnitsReceiveds++;
> +                int64_t duration, timeUs;
> +                mSessionDesc->getDurationUs(&duration);
> +                accessUnit->meta()->findInt64("timeUs", &timeUs);
> +                // we only check end-of-stream in the last 10 seconds.

nit: Capitalize the first letter.

@@ +1090,5 @@
> +                accessUnit->meta()->findInt64("timeUs", &timeUs);
> +                // we only check end-of-stream in the last 10 seconds.
> +                if (timeUs >= duration - 10) {
> +                    postEndOfStreamCheck(trackIndex);
> +                }

Also name a constant for this.
Do not use magic number.
Attachment #8595836 - Flags: review?(ettseng) → ui-review+
(Reporter)

Updated

3 years ago
Attachment #8595836 - Flags: ui-review+ → feedback+
(Assignee)

Comment 9

3 years ago
Created attachment 8596495 [details] [diff] [review]
Separate timer of access unit timeout and end-of-stream detection.

Hi Ethan, please review again. Thanks.
Attachment #8595836 - Attachment is obsolete: true
Attachment #8596495 - Flags: review?(ettseng)
(Assignee)

Comment 10

3 years ago
(In reply to Ethan Tseng [:ethan] from comment #8)
> Is there a reason to remove this code fragment?
> 
> /* missing code fragment */
> if (gIOService->IsOffline()) {	
>     sp<AMessage> reply = new AMessage(kWhatDisconnected, id());	
>     reply->setInt32("result", ERROR_CONNECTION_LOST);	
>     mConn->disconnect(reply);	
>     break;	
> }
> /* end of missing code */

I thought we had the mechanism to detect network disconnection, so we didn't need that code.
But now, on second thoughts, I think it's still useful because the network may disconnect right in the last 2 second range.
In the new patch, I keep that code but use disconnect() instead, because it will go to kWhatAbort and do the cleanup and send TEARDOWN.

I also revised the comment, and change the constant to 2 seconds. I think mistreating disconnection as end-of-stream in the last 2 seconds wouldn't harm user experience too much.
(Reporter)

Comment 11

3 years ago
Comment on attachment 8596495 [details] [diff] [review]
Separate timer of access unit timeout and end-of-stream detection.

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

Thanks for the patch! Looks good to me.
I just have a minor suggestion on naming, but you could choose to apply it or not on your own. :)

::: netwerk/protocol/rtsp/rtsp/RTSPConnectionHandler.h
@@ +50,5 @@
> +// stream has ended and abort.
> +static int64_t kAccessUnitTimeoutUs = 10000000ll;
> +
> +// We start the end-of-stream timer in the last 2 seconds.
> +static int64_t kPrepareForEndOfStreamUs = 2000000ll;

My suggested naming:
static int64_t kActivateEndOfStreamTimerUs = 2000000ll;

@@ +1095,5 @@
> +                mSessionDesc->getDurationUs(&duration);
> +                accessUnit->meta()->findInt64("timeUs", &timeUs);
> +
> +                // Start a timer to detect end-of-stream if close to the end.
> +                if (timeUs >= duration - kPrepareForEndOfStreamUs) {

if (timeUs >= duration - kActivateEndOfStreamTimerUs)
Attachment #8596495 - Flags: review?(ettseng) → review+
(Assignee)

Comment 12

3 years ago
Created attachment 8597076 [details] [diff] [review]
Separate timer of access unit timeout and end-of-stream detection.

I agree with your naming choice.

Waiting for try server results:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2c63b5ecbaf6
Attachment #8596495 - Attachment is obsolete: true
Attachment #8597076 - Flags: review+
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/6b935b2fd11c
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox40: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 2.2 S11 (1may)

Updated

3 years ago
blocking-b2g: --- → 2.0M+

Updated

3 years ago
Duplicate of this bug: 1158661

Updated

3 years ago
Blocks: 1054172
You need to log in before you can comment on or make changes to this bug.