1242845 - Make the computation of MediaDecoderStateMachine::HasLowUndecodedData() more accurate and consistent

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, defect)

Description

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

We should reference mDecodedVideoEndTime when calculating endOfDecodedVideoData to be consistent with the calculation of endOfDecodedAudioData. Also we should check {Audio,Video}Queue().IsFinished() instead of AtEndOfStream() since we won't be low in un-decoded data once all audio/video frames are decoded.

Comment 1

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

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

Comment 2

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

Attachment: MozReview Request: Bug 1242845 - Make the computation of MediaDecoderStateMachine::HasLowUndecodedData() more accurate and consistent. r=jya.

Review commit: https://reviewboard.mozilla.org/r/32405/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/32405/

Comment 3

[Jean-Yves Avenard [:jya]]

Comment on attachment 8712011 [details]
MozReview Request: Bug 1242845 - Make the computation of MediaDecoderStateMachine::HasLowUndecodedData() more accurate and consistent. r=jya.

https://reviewboard.mozilla.org/r/32405/#review29255

::: dom/media/MediaDecoderStateMachine.cpp:1882
(Diff revision 1)
> -  int64_t endOfDecodedAudioData = INT64_MAX;
> +                                  INT64_MAX;

I would prefer to avoid doubt by adding parenthesis around the test, and change the formatting like so:

int64_t endOfDecodedVideoData =
  (HasVideo() && !VideoQueue().IsFinished())
    ? mDecodedVideoEndTime
    : INT64_MAX;
    
as this is what we commonly use thorough the media code.

Comment 4

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

https://reviewboard.mozilla.org/r/32405/#review29255

> I would prefer to avoid doubt by adding parenthesis around the test, and change the formatting like so:
> 
> int64_t endOfDecodedVideoData =
>   (HasVideo() && !VideoQueue().IsFinished())
>     ? mDecodedVideoEndTime
>     : INT64_MAX;
>     
> as this is what we commonly use thorough the media code.

Will make changes as suggested. Thanks for the review.

Comment 5

[Pulsebot]

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

Comment 6

[Jean-Yves Avenard [:jya]]

(In reply to JW Wang [:jwwang] from comment #4)
> https://reviewboard.mozilla.org/r/32405/#review29255
> 
> > I would prefer to avoid doubt by adding parenthesis around the test, and change the formatting like so:
> > 
> > int64_t endOfDecodedVideoData =
> >   (HasVideo() && !VideoQueue().IsFinished())
> >     ? mDecodedVideoEndTime
> >     : INT64_MAX;
> >     
> > as this is what we commonly use thorough the media code.
> 
> Will make changes as suggested. Thanks for the review.

you didn't :(

rather disappointed.

Comment 7

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

The changeset is here:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b4eb8810725e

Isn't this what you want?

Comment 8

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

OK, I see. You want '?' and ':' in the front of the line instead of the back.

Comment 9

[Pulsebot]

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

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b4eb8810725e
https://hg.mozilla.org/mozilla-central/rev/a282f57d22b6

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b4eb8810725e
https://hg.mozilla.org/mozilla-central/rev/a282f57d22b6