Closed Bug 1085204 Opened 8 years ago Closed 8 years ago

VideoFrameContainer::SetCurrentFrame() doesn't respect aTargetTime

Categories

(Core :: Audio/Video, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: jwwang, Assigned: jwwang)

Details

Attachments

(1 file, 2 obsolete files)

VideoFrameContainer::SetCurrentFrame() doesn't respect |aTargetTime| by drawing the passed image as soon as possible. Therefore it is possible get a negative frame paint delay and fail the test at http://hg.mozilla.org/mozilla-central/file/ae1dfa192faf/content/media/test/test_reset_src.html#l47.

Should we just disable the test since there is no easy way to implement |aTargetTime| soon.
s/disable/todo/ to notice that we have a working item to be fixed.
Flags: needinfo?(roc)
My understanding is that the current video code shouldn't call SetCurrentFrame until aTargetTime has already passed. cpearce, is that true?
Flags: needinfo?(roc) → needinfo?(cpearce)
Basically yes. We use timers to wake up at the time for the next frame, so there could be slop due to timer delays.

aTargetTime is not scheduling the paint time of the frame, it's passed through to measure how late the frame is.
Flags: needinfo?(cpearce)
When |mScheduler->IsRealTime()| is true [1], it is possible to select a frame where |clock_time < frame->mTime|. Then |presTime| at [2] will be greater than |TimeStamp::Now()| and we will get a negative frame delay.

I am curious about when IsRealTime() will be true and what its purpose is.

[1] http://hg.mozilla.org/mozilla-central/file/f2d7d694aae5/content/media/MediaDecoderStateMachine.cpp#l2648
[2] http://hg.mozilla.org/mozilla-central/file/f2d7d694aae5/content/media/MediaDecoderStateMachine.cpp#l2704
1. Presentation time of video frame should take playback rate into account.
2. ScheduleStateMachine() should also take playback rate into account.
3. Update the way calculating video frame presentation time which doesn't require resync audio clock.

Try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=6b6d713054c2
Most green.
Assignee: nobody → jwwang
Status: NEW → ASSIGNED
Attachment #8508511 - Flags: review?(cpearce)
Comment on attachment 8508511 [details] [diff] [review]
1085204_fix_video_frame_presTime.patch

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

::: content/media/MediaDecoderStateMachine.cpp
@@ +2703,5 @@
>  
>    if (currentFrame) {
>      // Decode one frame and display it.
> +    TimeStamp presTime = nowTime +
> +        UsecsToDuration((currentFrame->mTime - clock_time) / mPlaybackRate);

Should we just say |presTime = nowTime| in real time mode for there is not timestamp in real time frames?
(In reply to Chris Pearce (:cpearce) from comment #3)
> aTargetTime is not scheduling the paint time of the frame, it's passed
> through to measure how late the frame is.

So we should never pass a future aTargetTime and it should always be smaller than TimeStamp::Now(), right?
Comment on attachment 8508511 [details] [diff] [review]
1085204_fix_video_frame_presTime.patch

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

::: content/media/MediaDecoderStateMachine.cpp
@@ +163,5 @@
>  // less than this are ignored, as they're assumed to be the result of
>  // instability in the duration estimation.
>  static const int64_t ESTIMATED_DURATION_FUZZ_FACTOR_USECS = USECS_PER_S / 2;
>  
> +static TimeDuration UsecsToDuration(double aUsecs) {

I'd prefer it if you didn't make this change, as normally we use int64_t for microseconds, and double for seconds, so it would be confusing.

@@ +2703,5 @@
>  
>    if (currentFrame) {
>      // Decode one frame and display it.
> +    TimeStamp presTime = nowTime +
> +        UsecsToDuration((currentFrame->mTime - clock_time) / mPlaybackRate);

No, the frame delay is supposed to be a measure of how late a frame is by the time it gets onto the screen, and the timestamps are still valid in realtime decoding.
Attachment #8508511 - Flags: review?(cpearce) → review+
Comment on attachment 8508511 [details] [diff] [review]
1085204_fix_video_frame_presTime.patch

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

::: content/media/MediaDecoderStateMachine.cpp
@@ +163,5 @@
>  // less than this are ignored, as they're assumed to be the result of
>  // instability in the duration estimation.
>  static const int64_t ESTIMATED_DURATION_FUZZ_FACTOR_USECS = USECS_PER_S / 2;
>  
> +static TimeDuration UsecsToDuration(double aUsecs) {

OK.

I was to save the double conversion in UsecsToDuration((currentFrame->mTime - clock_time) / mPlaybackRate). I think compiler should be smarter enough to inline this function and eliminate unnecessary conversion.

@@ +2703,5 @@
>  
>    if (currentFrame) {
>      // Decode one frame and display it.
> +    TimeStamp presTime = nowTime +
> +        UsecsToDuration((currentFrame->mTime - clock_time) / mPlaybackRate);

So the live stream frames will also carry valid timestamps. Then we should assert |presTime <= TimeStamp::Now()| here since the calculated presTime should never be a time in the future.
Hi Benjamin,

Once upon a time, I remembered I was told sometimes the mTime of real-time frames are all zeros. Can you help check that?
Flags: needinfo?(bechen)
(In reply to JW Wang [:jwwang] from comment #10)
> Hi Benjamin,
> 
> Once upon a time, I remembered I was told sometimes the mTime of real-time
> frames are all zeros. Can you help check that?

For current RTSP implementation, we set timestamp to zero for realtime rtsp streaming.
Because we think the timestamp is useless and we don't have total duration for the realtime stream. So we treat them as zero.

But since we introduced play-out delay for RTSP and now we are fixing some realtime streaming bugs, we are considering to add timestamp on realtime streaming frames. Once we add timestamp successfully, we might disable the isRealTime() flag in statemachine or remove the isRealTime() flag.
Flags: needinfo?(bechen)
I see. Then |presTime <= TimeStamp::Now()| should always hold even in the case of live stream where timestampes are zeros.
Attachment #8508511 - Attachment is obsolete: true
Attachment #8511766 - Flags: review+
Hi Jw, the patch didn't apply cleanly:

applying 1085204_fix_video_frame_presTime-v2.patch
unable to find 'content/media/MediaDecoderStateMachine.cpp' for patching
5 out of 5 hunks FAILED -- saving rejects to file content/media/MediaDecoderStateMachine.cpp.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir


could you take a look and maybe rebase, thanks!
Keywords: checkin-needed
They moved the folder content/media to dom/media. I will update the patch.
Fix file path since content/media is moved to dom/media.
Attachment #8511766 - Attachment is obsolete: true
Attachment #8512442 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/5aee2a79cbb3
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.