712738 - Clang Static Analysis: Assigned value is garbage or undefined in content/media/nsBuiltinDecoder.cpp

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Christian Holler (:decoder)]

The following report (in the URL field) has been generated by static analysis using Clang.

It would be good if someone familiar with the particular code could check if

- this is really a bug or a false positive
- and/or if it makes sense to adjust the code (even if there is not a real bug present, e.g. by adding a missing initialization).


In this particular report, the problem seems to be that the conditional statement in line 341 could be reached without | rightBound | being initialized at all. Even if this can't happen (which has to be checked), it would be good to initialize the doubles to default values unless there is some reason not to do so.

Comment 1

[Chris Pearce [:cpearce (Not reading bugmail)]]

Thanks. We can hit this case when playing a live stream and we seek past the last buffered range. We need to fix this.

Comment 2

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch v1

* Fix seek logic to clamp seek target to end of last buffered range if seek target is after the end of last buffered range on live streams.
* When playing streams for which we can't fnd a duration, we update nsBuiltinDecoderStateMachine::mEndTime in AdvanceFrame(), but if we're downloading data faster than playing it, mEndTime will thus lag far behind the actual end of downloaded data. So update mEndTime in nsBuiltinDecoderStateMachine::NotifyDataArrived() as more data comes in. We need to do this so that the seek target clamping in nsBuiltinDecoderStateMachine::Seek() clamps to the end of buffered data, rather than to the end of played data.

Comment 3

[Matthew Gregan [:kinetik]]

Comment on attachment 586288 [details] [diff] [review]
Patch v1

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

::: content/media/nsBuiltinDecoder.cpp
@@ +315,5 @@
>    }
>  
>    // If the position we want to seek to is not in a seekable range, we seek
>    // to the closest position in the seekable ranges instead . If two positions
>    // are equaly close, we seek to the closest position from the currentTime.

Nitpicks: mind fixing the "equaly" typo here?  And also the extra space near "instead .".  And "immediatly" in the comment above IsInRanges.

::: content/media/nsBuiltinDecoderStateMachine.cpp
@@ +1137,5 @@
> +  // since we haven't played the frame at the end of buffered data. So update
> +  // mEndTime here as new data is downloaded to prevent such a lag.
> +  nsTimeRanges buffered;
> +  if (mDecoder->IsInfinite() &&
> +      !mDecoder->IsSeekable() && 

IsInfinite() is false for seekable streams, so it shouldn't be necessary to test !IsSeekable().  And if we did have infinite seekable streams, I think it would make sense to update the end time using this logic anyway.

Comment 4

[Chris Pearce [:cpearce (Not reading bugmail)]]

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

Comment 5

[Ed Morley [:emorley]]

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