Closed Bug 1114434 Opened 5 years ago Closed 5 years ago

Fix logic in MediaQueue::Duration()

Categories

(Core :: Audio/Video, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38

People

(Reporter: jhao, Assigned: jhao)

References

Details

Attachments

(1 file, 2 obsolete files)

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()
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.
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?
Flags: needinfo?(cpearce)
(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;
(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.
(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 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...
Attachment #8540570 - Flags: review?(cpearce) → review?(jwwang)
Flags: needinfo?(cpearce)
Comment on attachment 8540570 [details] [diff] [review]
Reimplement MediaQueue::Duration() using a member mDuration

I will make another patch based on JW's suggestions.
Attachment #8540570 - Flags: review?(jwwang)
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.
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.
Attachment #8540570 - Attachment is obsolete: true
Attachment #8541162 - Flags: review?(jwwang)
Attachment #8541162 - Attachment is patch: true
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()) { ...
Attachment #8541162 - Flags: review?(jwwang) → review+
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.
Attachment #8541162 - Attachment is obsolete: true
Attachment #8541184 - Flags: review+
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.
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.
Those oranges are mostly related to streams...
No longer blocks: 1080461
Depends on: 1115505
Hi Jonathan,
Please try the patch in bug 1115505 to see if it fix your oranges. Thanks.
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
(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.
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.
Keywords: checkin-needed
Summary: Use FrameCount() to implement MediaQueue::Duration() → Fix logic in MediaQueue::Duration()
https://hg.mozilla.org/mozilla-central/rev/bee5bb286d47
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
You need to log in before you can comment on or make changes to this bug.