VIDEO: Cleanup VideoDecoder a bit

Functions and their comments now line up better

2 files changed, 193 insertions, 126 deletions

diff --git a/video/video_decoder.cpp b/video/video_decoder.cpp
index cf11649c10..14826642d7 100644
--- a/video/video_decoder.cpp
+++ b/video/video_decoder.cpp
@@ -413,6 +413,10 @@ Audio::Timestamp VideoDecoder::Track::getDuration() const {
 	return Audio::Timestamp(0, 1000);
 }
 
+bool VideoDecoder::VideoTrack::endOfTrack() const {
+	return getCurFrame() >= (getFrameCount() - 1);
+}
+
 uint32 VideoDecoder::FixedRateVideoTrack::getNextFrameStartTime() const {
 	if (endOfTrack() || getCurFrame() < 0)
 		return 0;
@@ -422,10 +426,6 @@ uint32 VideoDecoder::FixedRateVideoTrack::getNextFrameStartTime() const {
 	return time.toInt();
 }
 
-bool VideoDecoder::FixedRateVideoTrack::endOfTrack() const {
-	return getCurFrame() >= (getFrameCount() - 1);
-}
-
 Audio::Timestamp VideoDecoder::FixedRateVideoTrack::getDuration() const {
 	// Since Audio::Timestamp doesn't support a fractional frame rate, we're currently
 	// just converting to milliseconds.
diff --git a/video/video_decoder.h b/video/video_decoder.h
index abb9f8df20..5fba85358c 100644
--- a/video/video_decoder.h
+++ b/video/video_decoder.h
@@ -54,10 +54,14 @@ public:
 	VideoDecoder();
 	virtual ~VideoDecoder() {}
 
+	/////////////////////////////////////////
+	// Opening/Closing a Video
+	/////////////////////////////////////////
+
 	/**
 	 * Load a video from a file with the given name.
 	 *
-	 * A default implementation using loadStream is provided.
+	 * A default implementation using Common::File and loadStream is provided.
 	 *
 	 * @param filename	the filename to load
 	 * @return whether loading the file succeeded
@@ -68,6 +72,10 @@ public:
 	 * Load a video from a generic read stream. The ownership of the
 	 * stream object transfers to this VideoDecoder instance, which is
 	 * hence also responsible for eventually deleting it.
+	 *
+	 * Implementations of this function are required to call addTrack()
+	 * for each track in the video upon success.
+	 *
 	 * @param stream  the stream to load
 	 * @return whether loading the stream succeeded
 	 */
@@ -75,6 +83,9 @@ public:
 
 	/**
 	 * Close the active video stream and free any associated resources.
+	 *
+	 * All subclasses that need to close their own resources should still
+	 * call the base class' close() function at the start of their function.
 	 */
 	virtual void close();
 
@@ -83,87 +94,66 @@ public:
 	 */
 	bool isVideoLoaded() const;
 
-	/**
-	 * Returns the width of the video's frames.
-	 * @return the width of the video's frames
-	 */
-	virtual uint16 getWidth() const;
-
-	/**
-	 * Returns the height of the video's frames.
-	 * @return the height of the video's frames
-	 */
-	virtual uint16 getHeight() const;
 
-	/**
-	 * Get the pixel format of the currently loaded video.
-	 */
-	virtual Graphics::PixelFormat getPixelFormat() const;
+	/////////////////////////////////////////
+	// Playback Control
+	/////////////////////////////////////////
 
 	/**
-	 * Get the palette for the video in RGB format (if 8bpp or less).
+	 * Begin playback of the video.
+	 *
+	 * @note This has no effect is the video is already playing.
 	 */
-	const byte *getPalette();
+	void start();
 
 	/**
-	 * Returns if the palette is dirty or not.
+	 * Stop playback of the video.
+	 *
+	 * @note This will close() the video if it is not rewindable.
+	 * @note If the video is rewindable, the video will be rewound on the
+	 * next start() call unless rewind() or seek() is called before then.
 	 */
-	bool hasDirtyPalette() const { return _dirtyPalette; }
+	void stop();
 
 	/**
-	 * Returns the current frame number of the video.
-	 * @return the last frame decoded by the video
+	 * Returns if the video is currently playing or not.
+	 * @todo Differentiate this function from endOfVideo()
 	 */
-	int32 getCurFrame() const;
+	bool isPlaying() const { return _isPlaying; }
 
 	/**
-	 * Returns the number of frames in the video.
-	 * @return the number of frames in the video
+	 * Returns if a video is rewindable or not. The default implementation
+	 * polls each track for rewindability.
 	 */
-	uint32 getFrameCount() const;
+	virtual bool isRewindable() const;
 
 	/**
-	 * Returns the time position (in ms) of the current video.
-	 * This can be based on the "wall clock" time as determined by
-	 * OSystem::getMillis() or the current time of any audio track
-	 * running in the video, and takes pausing the video into account.
+	 * Rewind a video to its beginning.
 	 *
-	 * As such, it will differ from what multiplying getCurFrame() by
-	 * some constant would yield, e.g. for a video with non-constant
-	 * frame rate.
+	 * If the video is playing, it will continue to play. The default
+	 * implementation will rewind each track.
 	 *
-	 * Due to the nature of the timing, this value may not always be
-	 * completely accurate (since our mixer does not have precise
-	 * timing).
-	 */
-	uint32 getTime() const;
-
-	/**
-	 * Return the time (in ms) until the next frame should be displayed.
-	 */
-	uint32 getTimeToNextFrame() const;
-
-	/**
-	 * Check whether a new frame should be decoded, i.e. because enough
-	 * time has elapsed since the last frame was decoded.
-	 * @return whether a new frame should be decoded or not
+	 * @return true on success, false otherwise
 	 */
-	virtual bool needsUpdate() const;
+	virtual bool rewind();
 
 	/**
-	 * Decode the next frame into a surface and return the latter.
-	 * @return a surface containing the decoded frame, or 0
-	 * @note Ownership of the returned surface stays with the VideoDecoder,
-	 *       hence the caller must *not* free it.
-	 * @note this may return 0, in which case the last frame should be kept on screen
+	 * Returns if a video is seekable or not. The default implementation
+	 * polls each track for seekability.
 	 */
-	virtual const Graphics::Surface *decodeNextFrame();
+	virtual bool isSeekable() const;
 
 	/**
-	 * Returns if the video has finished playing or not.
-	 * @return true if the video has finished playing or if none is loaded, false otherwise
+	 * Seek to a given time in the video.
+	 *
+	 * If the video is playing, it will continue to play. The default
+	 * implementation will seek each track and must still be called
+	 * from any other implementation.
+	 *
+	 * @param time The time to seek to
+	 * @return true on success, false otherwise
 	 */
-	bool endOfVideo() const;
+	virtual bool seek(const Audio::Timestamp &time);
 
 	/**
 	 * Pause or resume the video. This should stop/resume any audio playback
@@ -184,103 +174,141 @@ public:
 	bool isPaused() const { return _pauseLevel != 0; }
 
 	/**
-	 * Get the current volume at which the audio in the video is being played
-	 * @return the current volume at which the audio in the video is being played
+	 * Set the time for this video to end at. At this time in the video,
+	 * all audio will stop and endOfVideo() will return true.
 	 */
-	byte getVolume() const { return _audioVolume; }
+	void setEndTime(const Audio::Timestamp &endTime);
 
 	/**
-	 * Set the volume at which the audio in the video should be played.
-	 * This setting remains until reset() is called (which may be called
-	 * from loadStream() or close()). The default volume is the maximum.
-	 *
-	 * @note This function calls updateVolume() by default.
-	 *
-	 * @param volume The volume at which to play the audio in the video
+	 * Get the stop time of the video (if not set, zero)
 	 */
-	void setVolume(byte volume);
+	Audio::Timestamp getEndTime() const { return _endTime; }
+
+
+	/////////////////////////////////////////
+	// Playback Status
+	/////////////////////////////////////////
 
 	/**
-	 * Get the current balance at which the audio in the video is being played
-	 * @return the current balance at which the audio in the video is being played
+	 * Returns if the video has reached the end or not.
+	 * @return true if the video has finished playing or if none is loaded, false otherwise
 	 */
-	int8 getBalance() const { return _audioBalance; }
+	bool endOfVideo() const;
 
 	/**
-	 * Set the balance at which the audio in the video should be played.
-	 * This setting remains until reset() is called (which may be called
-	 * from loadStream() or close()). The default balance is 0.
-	 *
-	 * @note This function calls updateBalance() by default.
-	 *
-	 * @param balance The balance at which to play the audio in the video
+	 * Returns the current frame number of the video.
+	 * @return the last frame decoded by the video
 	 */
-	void setBalance(int8 balance);
+	int32 getCurFrame() const;
 
 	/**
-	 * Returns if a video is rewindable or not. The default implementation
-	 * polls each track for rewindability.
+	 * Returns the number of frames in the video.
+	 * @return the number of frames in the video
 	 */
-	virtual bool isRewindable() const;
+	uint32 getFrameCount() const;
 
 	/**
-	 * Rewind a video to its beginning.
+	 * Returns the time position (in ms) of the current video.
+	 * This can be based on the "wall clock" time as determined by
+	 * OSystem::getMillis() or the current time of any audio track
+	 * running in the video, and takes pausing the video into account.
 	 *
-	 * If the video is playing, it will continue to play. The default
-	 * implementation will rewind each track.
+	 * As such, it will differ from what multiplying getCurFrame() by
+	 * some constant would yield, e.g. for a video with non-constant
+	 * frame rate.
 	 *
-	 * @return true on success, false otherwise
+	 * Due to the nature of the timing, this value may not always be
+	 * completely accurate (since our mixer does not have precise
+	 * timing).
 	 */
-	virtual bool rewind();
+	uint32 getTime() const;
+
+
+	/////////////////////////////////////////
+	// Video Info
+	/////////////////////////////////////////
 
 	/**
-	 * Returns if a video is seekable or not. The default implementation
-	 * polls each track for seekability.
+	 * Returns the width of the video's frames.
+	 *
+	 * By default, this finds the largest width between all of the loaded
+	 * tracks. However, a subclass may override this if it does any kind
+	 * of post-processing on it.
+	 *
+	 * @return the width of the video's frames
 	 */
-	virtual bool isSeekable() const;
+	virtual uint16 getWidth() const;
 
 	/**
-	 * Seek to a given time in the video.
+	 * Returns the height of the video's frames.
 	 *
-	 * If the video is playing, it will continue to play. The default
-	 * implementation will seek each track.
+	 * By default, this finds the largest height between all of the loaded
+	 * tracks. However, a subclass may override this if it does any kind
+	 * of post-processing on it.
 	 *
-	 * @param time The time to seek to
-	 * @return true on success, false otherwise
+	 * @return the height of the video's frames
 	 */
-	virtual bool seek(const Audio::Timestamp &time);
+	virtual uint16 getHeight() const;
 
 	/**
-	 * Begin playback of the video.
+	 * Get the pixel format of the currently loaded video.
+	 */
+	Graphics::PixelFormat getPixelFormat() const;
+
+	/**
+	 * Get the duration of the video.
 	 *
-	 * @note This has no effect is the video is already playing.
+	 * If the duration is unknown, this will return 0. If this is not
+	 * overriden, it will take the length of the longest track.
 	 */
-	void start();
+	virtual Audio::Timestamp getDuration() const;
+
+
+	/////////////////////////////////////////
+	// Frame Decoding
+	/////////////////////////////////////////
 
 	/**
-	 * Stop playback of the video.
+	 * Get the palette for the video in RGB format (if 8bpp or less).
 	 *
-	 * @note This will close() the video if it is not rewindable.
+	 * The palette's format is the same as PaletteManager's palette
+	 * (interleaved RGB values).
 	 */
-	void stop();
+	const byte *getPalette();
 
 	/**
-	 * Returns if the video is currently playing or not.
-	 * @todo Differentiate this function from endOfVideo()
+	 * Returns if the palette is dirty or not.
 	 */
-	bool isPlaying() const { return _isPlaying; }
+	bool hasDirtyPalette() const { return _dirtyPalette; }
 
 	/**
-	 * Get the duration of the video.
-	 *
-	 * If the duration is unknown, this will return 0.
+	 * Return the time (in ms) until the next frame should be displayed.
 	 */
-	virtual Audio::Timestamp getDuration() const;
+	uint32 getTimeToNextFrame() const;
 
 	/**
-	 * Add an audio track from a stream file.
+	 * Check whether a new frame should be decoded, i.e. because enough
+	 * time has elapsed since the last frame was decoded.
+	 * @return whether a new frame should be decoded or not
 	 */
-	bool addStreamFileTrack(const Common::String &baseName);
+	bool needsUpdate() const;
+
+	/**
+	 * Decode the next frame into a surface and return the latter.
+	 *
+	 * A subclass may override this, but must still call this function. As an
+	 * example, a subclass may do this to apply some global video scale to
+	 * individual track's frame.
+	 *
+	 * Note that this will call readNextPacket() internally first before calling
+	 * the next video track's decodeNextFrame() function.
+	 *
+	 * @return a surface containing the decoded frame, or 0
+	 * @note Ownership of the returned surface stays with the VideoDecoder,
+	 *       hence the caller must *not* free it.
+	 * @note this may return 0, in which case the last frame should be kept on screen
+	 */
+	virtual const Graphics::Surface *decodeNextFrame();
 
 	/**
 	 * Set the default high color format for videos that convert from YUV.
@@ -292,16 +320,48 @@ public:
 	 */
 	void setDefaultHighColorFormat(const Graphics::PixelFormat &format) { _defaultHighColorFormat = format; }
 
+
+	/////////////////////////////////////////
+	// Audio Control
+	/////////////////////////////////////////
+
 	/**
-	 * Set the time for this video to end at. At this time in the video,
-	 * all audio will stop and endOfVideo() will return true.
+	 * Get the current volume at which the audio in the video is being played
+	 * @return the current volume at which the audio in the video is being played
 	 */
-	void setEndTime(const Audio::Timestamp &endTime);
+	byte getVolume() const { return _audioVolume; }
 
 	/**
-	 * Get the stop time of the video (if not set, zero)
+	 * Set the volume at which the audio in the video should be played.
+	 * This setting remains until close() is called (which may be called
+	 * from loadStream()). The default volume is the maximum.
+	 *
+	 * @param volume The volume at which to play the audio in the video
 	 */
-	Audio::Timestamp getEndTime() const { return _endTime; }
+	void setVolume(byte volume);
+
+	/**
+	 * Get the current balance at which the audio in the video is being played
+	 * @return the current balance at which the audio in the video is being played
+	 */
+	int8 getBalance() const { return _audioBalance; }
+
+	/**
+	 * Set the balance at which the audio in the video should be played.
+	 * This setting remains until close() is called (which may be called
+	 * from loadStream()). The default balance is 0.
+	 *
+	 * @param balance The balance at which to play the audio in the video
+	 */
+	void setBalance(int8 balance);
+
+	/**
+	 * Add an audio track from a stream file.
+	 *
+	 * This calls SeekableAudioStream::openStreamFile() internally
+	 */
+	bool addStreamFileTrack(const Common::String &baseName);
+
 
 	// Future API
 	//void setRate(const Common::Rational &rate);
@@ -337,11 +397,18 @@ protected:
 
 		/**
 		 * Return if the track is rewindable.
+		 *
+		 * If a video is seekable, it does not need to implement this
+		 * for it to also be rewindable.
 		 */
 		virtual bool isRewindable() const;
 
 		/**
 		 * Rewind the video to the beginning.
+		 *
+		 * If a video is seekable, it does not need to implement this
+		 * for it to also be rewindable.
+		 *
 		 * @return true on success, false otherwise.
 		 */
 		virtual bool rewind();
@@ -394,6 +461,7 @@ protected:
 		virtual ~VideoTrack() {}
 
 		TrackType getTrackType() const  { return kTrackTypeVideo; }
+		virtual bool endOfTrack() const;
 
 		/**
 		 * Get the width of this track
@@ -458,7 +526,6 @@ protected:
 		FixedRateVideoTrack() {}
 		virtual ~FixedRateVideoTrack() {}
 
-		virtual bool endOfTrack() const;
 		uint32 getNextFrameStartTime() const;
 		virtual Audio::Timestamp getDuration() const;
 
@@ -540,7 +607,7 @@ protected:
 
 	/**
 	 * An AudioTrack that implements isRewindable() and rewind() using
-	 * the RewindableAudioStream API.
+	 * RewindableAudioStream.
 	 */
 	class RewindableAudioTrack : public AudioTrack {
 	public:
@@ -562,7 +629,7 @@ protected:
 
 	/**
 	 * An AudioTrack that implements isSeekable() and seek() using
-	 * the SeekableAudioStream API.
+	 * SeekableAudioStream.
 	 */
 	class SeekableAudioTrack : public AudioTrack {
 	public:
@@ -613,7 +680,7 @@ protected:
 	/**
 	 * Decode enough data for the next frame and enough audio to last that long.
 	 *
-	 * This function is used by the default decodeNextFrame() function. A subclass
+	 * This function is used by the decodeNextFrame() function. A subclass
 	 * of a Track may decide to just have its decodeNextFrame() function read
 	 * and decode the frame.
 	 */
@@ -629,7 +696,7 @@ protected:
 	/**
 	 * Whether or not getTime() will sync with a playing audio track.
 	 *
-	 * A subclass should override this to disable this feature.
+	 * A subclass can override this to disable this feature.
 	 */
 	virtual bool useAudioSync() const { return true; }