Closed Bug 1080464 Opened 10 years ago Closed 10 years ago

[RTSP] Live stream frames are not rendered

Categories

(Firefox OS Graveyard :: RTSP, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
2.1 S7 (24Oct)

People

(Reporter: jhao, Assigned: jhao, Mentored)

References

Details

Attachments

(1 file, 1 obsolete file)

When a live stream is played, the screen becomes green, or stays on the first frame. No new frames are rendered.
Blocks: 1054171
Blocks: 1080467
Blocks: 1080468
Assignee: nobody → jhao
Attached patch bug-1080464-fix.patch (obsolete) — Splinter Review
After applying this patch, RTSP live stream should be able to run. I have tested the live streams on Ethan's Darwin Streaming Server and the ones from https://bugzilla.mozilla.org/show_bug.cgi?id=1072070#c0
Attachment #8503889 - Flags: review?(ettseng)
Comment on attachment 8503889 [details] [diff] [review]
bug-1080464-fix.patch

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

MediaDecoderStateMachine contains general logic which is not specific to RTSP.
I would like to have Benjamin's support to review it.

::: content/media/MediaDecoderStateMachine.cpp
@@ +2707,5 @@
>      // Filter out invalid frames by checking the frame time. FrameTime could be
>      // zero if it's a initial frame.
>      int64_t frameTime = currentFrame->mTime - mStartTime;
> +    if (frameTime > 0  || (frameTime == 0 && mPlayDuration == 0) ||
> +        mScheduler->IsRealTime()) {

It looks to me the 2nd and 3rd conditions both represent the live stream case.
Do we really need two conditions to check the same thing?
Attachment #8503889 - Flags: review?(bechen)
Comment on attachment 8503889 [details] [diff] [review]
bug-1080464-fix.patch

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

::: netwerk/protocol/rtsp/rtsp/RTSPSource.cpp
@@ +230,5 @@
>      }
>  
>      int64_t duration = 0;
>      getDuration(&duration);
> +    MOZ_ASSERT(duration == 0 || playTimeUs < duration,

Can you explain why the duration is always not 0 here? Live stream's duration?
(In reply to Benjamin Chen [:bechen] from comment #3)
> Comment on attachment 8503889 [details] [diff] [review]
> bug-1080464-fix.patch
> 
> Review of attachment 8503889 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: netwerk/protocol/rtsp/rtsp/RTSPSource.cpp
> @@ +230,5 @@
> >      }
> >  
> >      int64_t duration = 0;
> >      getDuration(&duration);
> > +    MOZ_ASSERT(duration == 0 || playTimeUs < duration,
> 
> Can you explain why the duration is always not 0 here? Live stream's
> duration?

This assertion checks if duration is 0, i.e. a live stream, then everything's OK; if it's not a live stream, then play time must not exceed duration. I assumed live stream's duration is always zero, isn't it?
Comment on attachment 8503889 [details] [diff] [review]
bug-1080464-fix.patch

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

::: content/media/MediaDecoderStateMachine.cpp
@@ +2707,5 @@
>      // Filter out invalid frames by checking the frame time. FrameTime could be
>      // zero if it's a initial frame.
>      int64_t frameTime = currentFrame->mTime - mStartTime;
> +    if (frameTime > 0  || (frameTime == 0 && mPlayDuration == 0) ||
> +        mScheduler->IsRealTime()) {

After reading the code comment, I realize they represent different conditions.
Thus, the change looks fine to me.
Comment on attachment 8503889 [details] [diff] [review]
bug-1080464-fix.patch

The patch looks fine to me.
Benjamin, what do you think?
Attachment #8503889 - Flags: review?(ettseng) → review+
Attachment #8503889 - Flags: review?(bechen) → review+
Adding r=ettseng and r=bechen to commit message.
Attachment #8503889 - Attachment is obsolete: true
(In reply to Jonathan Hao [:jhao] from comment #7)
> https://tbpl.mozilla.org/?tree=Try&rev=b7c537481e10
This one is cancelled because I forgot to run tests.

The one with tests:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=f5a03dd5080d
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/2764dc71106f
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.1 S7 (24Oct)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: