Closed Bug 895753 Opened 8 years ago Closed 7 years ago

Rtsp: Support suspend and resume to media stream server

Categories

(Firefox OS Graveyard :: Gaia::Camera, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(blocking-b2g:1.4+, firefox28 wontfix, firefox29 fixed, b2g-v1.4 fixed)

RESOLVED FIXED
1.3 C1/1.4 S1(20dec)
blocking-b2g 1.4+
Tracking Status
firefox28 --- wontfix
firefox29 --- fixed
b2g-v1.4 --- fixed

People

(Reporter: vchang, Assigned: vchang)

References

Details

(Whiteboard: [FT:RIL])

Attachments

(1 file, 1 obsolete file)

When phone screen is turned off, we should close the connection between Rtsp client and media server to save power. We should resume the connection after phone screen is turned on.
Have we fixed this bug? If not, is it okay for you to mark it ask 1.3+?
Flags: needinfo?(vchang)
Assignee: nobody → vchang
Flags: needinfo?(vchang)
blocking-b2g: --- → 1.3?
blocking-b2g: 1.3? → 1.3+
Target Milestone: --- → 1.3 Sprint 4 - 11/8
Whiteboard: [FT:RIL]
The save power when receiving suspend request, we should close the Rtsp connection to the Rtsp server, and reconnect to the server and send play request with correct timestamp when receiving the resume request.
Target Milestone: 1.3 Sprint 4 - 11/8 → 1.3 Sprint 6 - 12/6
Attached patch Patch v1.0 (obsolete) — Splinter Review
Instead of getting the start time from audio/video frame, this patch set the start time to zero and prevents sending play request with out of range start time. We can't get the first frame because because it is not kept in the buffer.
Attachment #8337678 - Flags: review?(sworkman)
Comment on attachment 8337678 [details] [diff] [review]
Patch v1.0

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

::: netwerk/protocol/rtsp/rtsp/RTSPSource.cpp
@@ +213,5 @@
> +    if (playTimeUs < duration) {
> +      LOGI("performPlay : duration=%lld playTimeUs=%lld", duration, playTimeUs);
> +      mState = PLAYING;
> +      mHandler->play(playTimeUs);
> +    }

If playTimeUS >= duration there is no notification that it failed. How does this work with the decoder state machine or JS events like seeking or seeked etc? What is the expectation if a play or seek cannot happen?

See HTMLMediaElement::SeekStarted and HTMLMediaElement::SeekCompleted.
> @@ +213,5 @@
> > +    if (playTimeUs < duration) {
> > +      LOGI("performPlay : duration=%lld playTimeUs=%lld", duration, playTimeUs);
> > +      mState = PLAYING;
> > +      mHandler->play(playTimeUs);
> > +    }
> 
> If playTimeUS >= duration there is no notification that it failed. How does
> this work with the decoder state machine or JS events like seeking or seeked
> etc? What is the expectation if a play or seek cannot happen?
> 
> See HTMLMediaElement::SeekStarted and HTMLMediaElement::SeekCompleted.

Thanks for your help for reviewing the patch during Thanks Giving.
I think the user may not expect to receive an internal error notification when seek or play time is out of the range. Decoder thread should detect this error and prevent sending the invalid seek or play command to RtspController. RtspController should also prevent sending the invalid seek or play command to server. Or the server may response with teardown message to close the connection. Not sure if we can use onDisConnected callback with "seek or play is out of range reason" to notify the decoder thread that the seek command has something wrong ? But for the time being, it may be good enough to just skip that command.
(In reply to Vincent Chang[:vchang] from comment #5)

> Thanks for your help for reviewing the patch during Thanks Giving.

It was just before Thanksgiving, so no worries :)

> I think the user may not expect to receive an internal error notification
> when seek or play time is out of the range. 

I was thinking more of the return value, or an assertion.

> Decoder thread should detect
> this error and prevent sending the invalid seek or play command to
> RtspController. RtspController should also prevent sending the invalid seek
> or play command to server. Or the server may response with teardown message
> to close the connection. Not sure if we can use onDisConnected callback with
> "seek or play is out of range reason" to notify the decoder thread that the
> seek command has something wrong ? But for the time being, it may be good
> enough to just skip that command.

I see in MediaDecoderStateMachine::Seek that seek time is bounded - it is changed to fit within the start and end times of the media presentation. So, this should mean that RtspSource::performPlay/Seek should be protected and should never receive an out of bounds time, right? If this is the assumption, then I think stopping the seek/play from being sent to the server is a good way to protect the code, but we also need some notification in the logs that things have gone wrong. I would like you to add an NS_ASSERTION at least, if not a MOZ_ASSERT - if it should NEVER happen, I would lean towards MOZ_ASSERT. Both types of assertion are debug only.

Something like changing:

  if (playTimeUs < duration) { ...

to 

  MOZ_ASSERT(playTimeUs < duration, "Should never receive an out of bounds time!");
  if (playTimeUs >= duration) {
    return;
  }
  LOGI("performPlay ...
  ...

This way, in debug builds we fail and crash, and it might help catch failures in automated testing more quickly. And in release builds, the code is very clear that we do not continue in the error case. You don't need to add a comment, because the string in MOZ_ASSERT should be enough.

From your comment, I think it's safe to assume that "seeking" and "seeked" events are taken care of by the state machine and decoder, so we don't need to worry about them.

Let me know what you think about this.
(In reply to Steve Workman [:sworkman] from comment #6)
> (In reply to Vincent Chang[:vchang] from comment #5)
> 
> > Thanks for your help for reviewing the patch during Thanks Giving.
> 
> It was just before Thanksgiving, so no worries :)
> 
> > I think the user may not expect to receive an internal error notification
> > when seek or play time is out of the range. 
> 
> I was thinking more of the return value, or an assertion.
> 
> > Decoder thread should detect
> > this error and prevent sending the invalid seek or play command to
> > RtspController. RtspController should also prevent sending the invalid seek
> > or play command to server. Or the server may response with teardown message
> > to close the connection. Not sure if we can use onDisConnected callback with
> > "seek or play is out of range reason" to notify the decoder thread that the
> > seek command has something wrong ? But for the time being, it may be good
> > enough to just skip that command.
> 
> I see in MediaDecoderStateMachine::Seek that seek time is bounded - it is
> changed to fit within the start and end times of the media presentation. So,
> this should mean that RtspSource::performPlay/Seek should be protected and
> should never receive an out of bounds time, right? If this is the
> assumption, then I think stopping the seek/play from being sent to the
> server is a good way to protect the code, but we also need some notification
> in the logs that things have gone wrong. I would like you to add an
> NS_ASSERTION at least, if not a MOZ_ASSERT - if it should NEVER happen, I
> would lean towards MOZ_ASSERT. Both types of assertion are debug only.
> 
> Something like changing:
> 
>   if (playTimeUs < duration) { ...
> 
> to 
> 
>   MOZ_ASSERT(playTimeUs < duration, "Should never receive an out of bounds
> time!");
>   if (playTimeUs >= duration) {
>     return;
>   }
>   LOGI("performPlay ...
>   ...
> 
> This way, in debug builds we fail and crash, and it might help catch
> failures in automated testing more quickly. And in release builds, the code
> is very clear that we do not continue in the error case. You don't need to
> add a comment, because the string in MOZ_ASSERT should be enough.
> 
> From your comment, I think it's safe to assume that "seeking" and "seeked"
> events are taken care of by the state machine and decoder, so we don't need
> to worry about them.
> 
> Let me know what you think about this.

I totally agree with your comment. 
So I'll update the patch and add the notification message. 
BTW, FindStartTime() in RtspOmxReader.h in the attached patch seems duplicated with the patch in Bug 938512. Maybe I should remove this part in this patch ?
This should not be the 1.3+ for a moment. The reason is that when the video app is launched via html a tag <a src=rtsp://....>  or  url bar case, the video app will be terminated when user presses the home key. So there is no resume case right now.
blocking-b2g: 1.3+ → 1.3?
(In reply to Vincent Chang[:vchang] from comment #7)

> I totally agree with your comment. 
> So I'll update the patch and add the notification message. 

Sounds good.

> BTW, FindStartTime() in RtspOmxReader.h in the attached patch seems
> duplicated with the patch in Bug 938512. Maybe I should remove this part in
> this patch ?

Best to talk with Ethan and harmonize your patches - maybe you need to have one patch dependent on the other.
Vincent,

Do we have an ETA for the fix?

If we land in master before 12/9 we don't need an uplift explicitly.
Component: General → Gaia::Camera
Flags: needinfo?(vchang)
RTSP not committed in 1.3.
blocking-b2g: 1.3? → -
blocking-b2g: - → madai?
Depends on: 938512
Flags: needinfo?(vchang)
Attached patch Patch v1.1Splinter Review
Update the patch to address the review commands.
Attachment #8337678 - Attachment is obsolete: true
Attachment #8337678 - Flags: review?(sworkman)
Attachment #8343635 - Flags: review?(sworkman)
Target Milestone: 1.3 Sprint 6 - 12/6 → ---
Comment on attachment 8343635 [details] [diff] [review]
Patch v1.1

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

Thanks Vincent! Looks good, r=me.
Attachment #8343635 - Flags: review?(sworkman) → review+
https://hg.mozilla.org/mozilla-central/rev/180befb0718e
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
blocking-b2g: madai? → 1.4+
Target Milestone: --- → 1.3 C1/1.4 S1(20dec)
You need to log in before you can comment on or make changes to this bug.