1114434 - Fix logic in MediaQueue::Duration()

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Jonathan Hao (inactive) [:jhao]]

In MediaQueue::Duration(), the current implementation returns the difference of mTime of the first and last element. However, this would be less than the real duration by one element's length, since the real duration should be the last element's end time minus the first element's start time.

Another problem is that RTSP live streams may have no time stamps, and all it's mTime are 0. Then the Duration() will always returns 0.

A solution is to use FrameCount() and FramesToUsecs to implement the Duration()

Comment 1

[Chris Pearce [:cpearce (GMT+13)]]

Could we instead maintain a member of MediaQueue, mDuration, and when we call MediaQueue::Push(MediaData*m) we add m->Duration() to MediaQueue::mDuration, and when we call MediaQueue::Pop() we subtract the duration of the sample being popped. Thus MediaQueue::Duration() therefore returns the sum of all the durations of the samples in the queue.

Comment 2

[Jonathan Hao (inactive) [:jhao]]

Hi Chris. I think this is a good Idea.

However, if there's discontinuity in the queue, the sum of durations (or my previous proposed FrameCount()) may not be the same as the time difference between the first and the last element.

Do you think that this problem is significant, or we can just ignore it?

Comment 3

[Jonathan Hao (inactive) [:jhao]]

Attachment: Reimplement MediaQueue::Duration() using a member mDuration

Comment 4

[JW Wang [:jwwang] [:jw_wang]]

(In reply to Jonathan Hao [:jhao] from comment #0)
> In MediaQueue::Duration(), the current implementation returns the difference
> of mTime of the first and last element. However, this would be less than the
> real duration by one element's length, since the real duration should be the
> last element's end time minus the first element's start time.
It looks like we should return |last->mTime - first->mTime + last->mDuration| for MediaQueue::Duration().

> Another problem is that RTSP live streams may have no time stamps, and all
> it's mTime are 0. Then the Duration() will always returns 0.
Is it possible to populate MediaData with valid time stamps? E.g.
firstSample->mTime = 0;
secondSample->mTime = firstSample->mTime + firstSample->mDuration;
thirdSample->mTime = secondSample->mTime + secondSample->mDuration;

Comment 5

[Jonathan Hao (inactive) [:jhao]]

(In reply to JW Wang [:jwwang] from comment #4)
> It looks like we should return |last->mTime - first->mTime +
> last->mDuration| for MediaQueue::Duration().
You're right. Another similar choice is |last->GetEndTime() - first->mTime|

> Is it possible to populate MediaData with valid time stamps? E.g.
> firstSample->mTime = 0;
> secondSample->mTime = firstSample->mTime + firstSample->mDuration;
> thirdSample->mTime = secondSample->mTime + secondSample->mDuration;
Yeah, this should ensure the mTime of samples in the queue at the same time is relatively correct to each other. However, if the queue becomes empty at some time, there is no previous sample, but we could perhaps store the latest mTime so far as a data member.

Comment 6

[JW Wang [:jwwang] [:jw_wang]]

(In reply to Jonathan Hao [:jhao] from comment #5)
> Yeah, this should ensure the mTime of samples in the queue at the same time
> is relatively correct to each other. However, if the queue becomes empty at
> some time, there is no previous sample, but we could perhaps store the
> latest mTime so far as a data member.

I think we can start with zero again since we just want to provide valid data for the calculation of MediaQueue::Duration.

Comment 7

[Chris Pearce [:cpearce (GMT+13)]]

Comment on attachment 8540570 [details] [diff] [review]
Reimplement MediaQueue::Duration() using a member mDuration

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

JW can take over review here, I'll be on PTO shortly...

Comment 8

[Jonathan Hao (inactive) [:jhao]]

Comment on attachment 8540570 [details] [diff] [review]
Reimplement MediaQueue::Duration() using a member mDuration

I will make another patch based on JW's suggestions.

Comment 9

[Benjamin Chen [:bechen]]

Comment on attachment 8540570 [details] [diff] [review]
Reimplement MediaQueue::Duration() using a member mDuration

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

There is a  |inline void Empty() | function in MediaQueue.h, maybe you should do something there too.
And please note that on b2g platform, the video frame has only mTime and the duration is 1.

Comment 10

[Jonathan Hao (inactive) [:jhao]]

Attachment: Fix logic in MediaQueue::Duration()

I think I'll just fix the part that the last duration is missed. As for the lack of mTime for RTSP, I'll file another bug to address it.

Comment 11

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8541162 [details] [diff] [review]
Fix logic in MediaQueue::Duration()

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

::: dom/media/MediaQueue.h
@@ +108,5 @@
>  
>    // Returns the approximate number of microseconds of items in the queue.
>    int64_t Duration() {
>      ReentrantMonitorAutoEnter mon(mReentrantMonitor);
> +    if (GetSize() < 1) {

Just say if (Empty()) { ...

Comment 12

[Jonathan Hao (inactive) [:jhao]]

Attachment: Fix logic in MediaQueue::Duration()

MediaQueue::Empty() is to empty the queue, instead of asking it if it's empty, but I'll change it to GetSize() == 0 to make it more readable.

Comment 13

[Benjamin Chen [:bechen]]

Comment on attachment 8541184 [details] [diff] [review]
Fix logic in MediaQueue::Duration()

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

::: dom/media/MediaQueue.h
@@ +105,5 @@
>      ReentrantMonitorAutoEnter mon(mReentrantMonitor);
>      mEndOfStream = true;
>    }
>  
>    // Returns the approximate number of microseconds of items in the queue.

Please modify the comment as well because we return an accurate value.

Comment 14

[Jonathan Hao (inactive) [:jhao]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8ba82f496e49

I didn't expect so much tests to fail with this small change. I've retriggered the failed ones, and will see what happened.

Comment 15

[JW Wang [:jwwang] [:jw_wang]]

Those oranges are mostly related to streams...

Comment 16

[JW Wang [:jwwang] [:jw_wang]]

Hi Jonathan,
Please try the patch in bug 1115505 to see if it fix your oranges. Thanks.

Comment 17

[Jonathan Hao (inactive) [:jhao]]

It's been running on try server, looking good so far.
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=e0eceb03330a

I also ran |./mach mochitest-plain| on my linux. Only these five tests failed

The following tests failed:
78838 INFO TEST-UNEXPECTED-FAIL | dom/browser-element/mochitest/test_browserElement_oop_SetInputMethodActive.html | Test timed out. - expected PASS
78839 INFO TEST-UNEXPECTED-FAIL | dom/browser-element/mochitest/test_browserElement_oop_SetInputMethodActive.html | [SimpleTest.finish()] waitForFocus() was called a different number of times from the number of callbacks run.  Maybe the test terminated prematurely -- be sure to use SimpleTest.waitForExplicitFinish(). - got 1, expected 0
78840 INFO TEST-UNEXPECTED-FAIL | dom/browser-element/mochitest/test_browserElement_oop_SetInputMethodActive.html | [SimpleTest.finish()] waitForFocus() was called a different number of times from the number of callbacks run.  Maybe the test terminated prematurely -- be sure to use SimpleTest.waitForExplicitFinish(). - Result logged after SimpleTest.finish()
78841 INFO TEST-UNEXPECTED-FAIL | dom/canvas/test/test_canvas_focusring.html | button 1 should not be focused - expected PASS
78842 INFO TEST-UNEXPECTED-FAIL | dom/canvas/test/test_canvas_focusring.html | focus of button 1 is not drawn - expected PASS

Comment 18

[Jonathan Hao (inactive) [:jhao]]

(In reply to Jonathan Hao [:jhao] from comment #17)
> It's been running on try server, looking good so far.
> https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=e0eceb03330a

There's one test failing 100% only on Windows 8 debug build.
dom/media/test/test_eme_playback.html | [00:29:49.996] gizmo-frag-cencinit.mp4-1 playing event should have fired - expected PASS

I'm not sure why it happened.

Comment 19

[Jonathan Hao (inactive) [:jhao]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5ff6c5c40e04

The error seems to be caused by other bugs, and it's fixed now. We're ready to check in.

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bee5bb286d47

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/bee5bb286d47