Closed Bug 1357987 Opened 3 years ago Closed 3 years ago

Make MediaDecoder::DEFAULT_NEXT_FRAME_AVAILABLE_BUFFERED a TimeUnit

Categories

(Core :: Audio/Video: Playback, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: jwwang, Assigned: jwwang)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

We want to avoid ambiguous int64_t for microseconds whenever possible.
Assignee: nobody → jwwang
Blocks: 1357986
Priority: -- → P3
Attachment #8860748 - Flags: review?(kaku)
Comment on attachment 8860748 [details]
Bug 1357987 - make MediaDecoder::DEFAULT_NEXT_FRAME_AVAILABLE_BUFFERED a TimeUnit to avoid ambiguous int64_t for microseconds.

https://reviewboard.mozilla.org/r/132672/#review135558

The original code is:
```
static const int DEFAULT_NEXT_FRAME_AVAILABLE_BUFFERED = 250000;

... currentPosition + TimeUnit::FromMicroseconds(DEFAULT_NEXT_FRAME_AVAILABLE_BUFFERED));
```

The new code is:
```
static constexpr auto DEFAULT_NEXT_FRAME_AVAILABLE_BUFFERED = media::TimeUnit::FromMicroseconds(250);

... currentPosition + DEFAULT_NEXT_FRAME_AVAILABLE_BUFFERED);
```

The original one is 250000 ms, and the new one is 250 ms, I think they are different, right? but, which one do we want?
Comment on attachment 8860748 [details]
Bug 1357987 - make MediaDecoder::DEFAULT_NEXT_FRAME_AVAILABLE_BUFFERED a TimeUnit to avoid ambiguous int64_t for microseconds.

https://reviewboard.mozilla.org/r/132672/#review135558

You are right. I will fix the code!
Comment on attachment 8860748 [details]
Bug 1357987 - make MediaDecoder::DEFAULT_NEXT_FRAME_AVAILABLE_BUFFERED a TimeUnit to avoid ambiguous int64_t for microseconds.

https://reviewboard.mozilla.org/r/132672/#review135606

::: dom/media/MediaDecoder.h:588
(Diff revision 2)
>    // Media data resource.
>    RefPtr<MediaResource> mResource;
>  
>    // Amount of buffered data ahead of current time required to consider that
>    // the next frame is available.
>    // An arbitrary value of 250ms is used.

So, the comment should be 250 seconds?
Attachment #8860748 - Flags: review?(kaku) → review+
Comment on attachment 8860748 [details]
Bug 1357987 - make MediaDecoder::DEFAULT_NEXT_FRAME_AVAILABLE_BUFFERED a TimeUnit to avoid ambiguous int64_t for microseconds.

https://reviewboard.mozilla.org/r/132672/#review135606

> So, the comment should be 250 seconds?

The comment is right. 250000us equals 250ms.
Thanks for the review!
Blocks: 1245019
No longer blocks: 1357986
Pushed by jwwang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/40c02952a836
make MediaDecoder::DEFAULT_NEXT_FRAME_AVAILABLE_BUFFERED a TimeUnit to avoid ambiguous int64_t for microseconds. r=kaku
https://hg.mozilla.org/mozilla-central/rev/40c02952a836
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.