Last Comment Bug 712738 - Clang Static Analysis: Assigned value is garbage or undefined in content/media/nsBuiltinDecoder.cpp
: Clang Static Analysis: Assigned value is garbage or undefined in content/medi...
Product: Core
Classification: Components
Component: Audio/Video (show other bugs)
: Trunk
: All All
-- normal (vote)
: mozilla12
Assigned To: Chris Pearce (:cpearce)
: Maire Reavy [:mreavy] Please needinfo me
Depends on:
Blocks: clang-based-analysis
  Show dependency treegraph
Reported: 2011-12-21 12:23 PST by Christian Holler (:decoder)
Modified: 2012-01-06 15:51 PST (History)
4 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Patch v1 (6.71 KB, patch)
2012-01-05 17:31 PST, Chris Pearce (:cpearce)
kinetik: review+
Details | Diff | Splinter Review

Description User image Christian Holler (:decoder) 2011-12-21 12:23:33 PST
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 User image Chris Pearce (:cpearce) 2011-12-21 14:43:57 PST
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 User image Chris Pearce (:cpearce) 2012-01-05 17:31:19 PST
Created attachment 586288 [details] [diff] [review]
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 User image Matthew Gregan [:kinetik] 2012-01-05 19:35:16 PST
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 5 User image Ed Morley [:emorley] 2012-01-06 15:51:56 PST

Note You need to log in before you can comment on or make changes to this bug.