Closed
Bug 1080464
Opened 10 years ago
Closed 10 years ago
[RTSP] Live stream frames are not rendered
Categories
(Firefox OS Graveyard :: RTSP, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
2.1 S7 (24Oct)
People
(Reporter: jhao, Assigned: jhao, Mentored)
References
Details
Attachments
(1 file, 1 obsolete file)
2.56 KB,
patch
|
Details | Diff | Splinter Review |
When a live stream is played, the screen becomes green, or stays on the first frame. No new frames are rendered.
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → jhao
Assignee | ||
Comment 1•10 years ago
|
||
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 2•10 years ago
|
||
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 3•10 years ago
|
||
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?
Assignee | ||
Comment 4•10 years ago
|
||
(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 5•10 years ago
|
||
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 6•10 years ago
|
||
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+
Updated•10 years ago
|
Attachment #8503889 -
Flags: review?(bechen) → review+
Assignee | ||
Comment 7•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=b7c537481e10
Assignee | ||
Comment 8•10 years ago
|
||
Adding r=ettseng and r=bechen to commit message.
Attachment #8503889 -
Attachment is obsolete: true
Assignee | ||
Comment 9•10 years ago
|
||
(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
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 10•10 years ago
|
||
https://hg.mozilla.org/integration/b2g-inbound/rev/2764dc71106f
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.
Description
•