SCI32: Clean up Video32

* Rewrap comments to 80 columns
* Clarify comments where appropriate

engines/sci/graphics/video32.cpp

@@ -63,8 +63,8 @@ bool VideoPlayer::open(const Common::String &fileName) {
 	}
 
 #ifndef USE_RGB_COLOR
-	// KQ7 2.00b videos are compressed in 24bpp Cinepak, so cannot play on
-	// a system with no RGB support
+	// KQ7 2.00b videos are compressed in 24bpp Cinepak, so cannot play on a
+	// system with no RGB support
 	if (_decoder->getPixelFormat().bytesPerPixel != 1) {
 		void showScummVMDialog(const Common::String &message);
 		showScummVMDialog(Common::String::format(_("Cannot play back %dbpp video on a system with maximum color depth of 8bpp"), _decoder->getPixelFormat().bpp()));
@@ -117,8 +117,8 @@ bool VideoPlayer::endHQVideo() {
 }
 
 VideoPlayer::EventFlags VideoPlayer::playUntilEvent(const EventFlags flags, const uint32 maxSleepMs) {
-	// Flushing all the keyboard and mouse events out of the event manager
-	// keeps events queued from before the start of playback from accidentally
+	// Flushing all the keyboard and mouse events out of the event manager keeps
+	// events queued from before the start of playback from accidentally
 	// activating a video stop flag
 	_eventMan->flushEvents();
 
@@ -201,11 +201,10 @@ void VideoPlayer::submitPalette(const uint8 palette[256 * 3]) const {
 	g_system->getPaletteManager()->setPalette(palette, 0, 256);
 
 	// KQ7 1.x has videos encoded using Microsoft Video 1 where palette 0 is
-	// white and 255 is black, which is basically the opposite of DOS/Win
-	// SCI palettes. So, when drawing to an 8bpp hwscreen, whenever a new
-	// palette is seen, the screen must be re-filled with the new black
-	// entry to ensure areas outside the video are always black and not some
-	// other color
+	// white and 255 is black, which is basically the opposite of DOS/Win SCI
+	// palettes. So, when drawing to an 8bpp hwscreen, whenever a new palette is
+	// seen, the screen must be re-filled with the new black entry to ensure
+	// areas outside the video are always black and not some other color
 	for (int color = 0; color < 256; ++color) {
 		if (palette[0] == 0 && palette[1] == 0 && palette[2] == 0) {
 			g_system->fillScreen(color);
@@ -231,7 +230,8 @@ void VideoPlayer::renderFrame(const Graphics::Surface &nextFrame) const {
 	if (_decoder->getWidth() != _drawRect.width() || _decoder->getHeight() != _drawRect.height()) {
 		Graphics::Surface *const unscaledFrame(convertedFrame);
 		// TODO: The only reason TransparentSurface is used here because it is
-		// where common scaler code is right now.
+		// where common scaler code is right now, which should just be part of
+		// Graphics::Surface (or some free functions).
 		const Graphics::TransparentSurface tsUnscaledFrame(*unscaledFrame);
 #ifdef USE_RGB_COLOR
 		if (_hqVideoMode) {
@@ -315,9 +315,9 @@ void SEQPlayer::play(const Common::String &fileName, const int16 numTicks, const
 	const int16 scaledWidth = (_decoder->getWidth() * Ratio(screenWidth, scriptWidth)).toInt();
 	const int16 scaledHeight = (_decoder->getHeight() * Ratio(screenHeight, scriptHeight)).toInt();
 
-	// Normally we would use the coordinates passed into the play function
-	// to position the video, but since we are scaling the video (which SSCI
-	// did not do), the coordinates are not correct. Since videos are always
+	// Normally we would use the coordinates passed into the play function to
+	// position the video, but since we are scaling the video (which SSCI did
+	// not do), the coordinates are not correct. Since videos are always
 	// intended to play in the center of the screen, we just recalculate the
 	// origin here.
 	_drawRect.left = (screenWidth - scaledWidth) / 2;
@@ -449,9 +449,9 @@ AVIPlayer::IOStatus AVIPlayer::play(const int16 from, const int16 to, const int1
 }
 
 AVIPlayer::EventFlags AVIPlayer::playUntilEvent(const EventFlags flags, const uint32 maxSleepMs) {
-	// NOTE: In SSCI, whether or not a video could be skipped was controlled by
-	// game scripts; here, we always allow skipping video with the mouse or
-	// escape key, to improve the user experience
+	// In SSCI, whether or not a video could be skipped was controlled by game
+	// scripts; here, we always allow skipping video with the mouse or escape
+	// key, to improve the user experience
 	return VideoPlayer::playUntilEvent(flags | kEventFlagMouseDown | kEventFlagEscapeKey, maxSleepMs);
 }
 
@@ -681,8 +681,8 @@ VMDPlayer::EventFlags VMDPlayer::kernelPlayUntilEvent(const EventFlags flags, co
 
 VMDPlayer::EventFlags VMDPlayer::playUntilEvent(const EventFlags flags, const uint32) {
 	if (flags & kEventFlagReverse) {
-		// NOTE: This flag may not work properly since SSCI does not care
-		// if a video has audio, but the VMD decoder does.
+		// This flag may not work properly since SSCI does not care if a video
+		// has audio, but the VMD decoder does.
 		warning("VMD reverse playback flag was set. Please report this event to the bug tracker");
 		const bool success = _decoder->setReverse(true);
 		assert(success);
@@ -838,10 +838,10 @@ void VMDPlayer::submitPalette(const uint8 rawPalette[256 * 3]) const {
 	if (_isComposited) {
 		SciBitmap *bitmap = _segMan->lookupBitmap(_bitmapId);
 		bitmap->setPalette(palette);
-		// NOTE: SSCI calls updateScreenItem and frameOut here, but this should
-		// not be necessary in ScummVM since the new palette gets submitted
-		// before the next frame is rendered, and the frame rendering call will
-		// perform the same operations.
+		// SSCI calls updateScreenItem and frameOut here, but this is not
+		// necessary in ScummVM since the new palette gets submitted before the
+		// next frame is rendered, and the frame rendering call will perform the
+		// same operations.
 	} else {
 		g_sci->_gfxPalette32->submit(palette);
 		g_sci->_gfxPalette32->updateForFrame();
@@ -931,7 +931,7 @@ void VMDPlayer::initComposited() {
 		_screenItem->_drawBlackLines = true;
 	}
 
-	// NOTE: There was code for positioning the screen item using insetRect
+	// In SSCI, there was code for positioning the screen item using insetRect
 	// here, but none of the game scripts seem to use this functionality.
 
 	g_sci->_gfxFrameout->addScreenItem(*_screenItem);
@@ -1016,9 +1016,8 @@ void VMDPlayer::setPlane(const int16 priority, const reg_t planeId) {
 
 void VMDPlayer::restrictPalette(const uint8 startColor, const int16 endColor) {
 	_startColor = startColor;
-	// At least GK2 sends 256 as the end color, which is wrong,
-	// but works in the original engine as the storage size is 4 bytes
-	// and used values are clamped to 0-255
+	// At least GK2 sends 256 as the end color, which is wrong, but works in
+	// SSCI as the storage size is 4 bytes and used values are clamped to 0-255
 	_endColor = MIN<int16>(255, endColor);
 }
 

engines/sci/graphics/video32.h

@@ -331,7 +331,7 @@ class VMDPlayer : public VideoPlayer {
 	 */
 	VMDStatus getStatus() const;
 
-	// NOTE: Was WaitForEvent in SSCI
+	// Was WaitForEvent in SSCI
 	EventFlags kernelPlayUntilEvent(const EventFlags flags, const int16 lastFrameNo, const int16 yieldInterval);
 
 private:
@@ -347,7 +347,8 @@ class VMDPlayer : public VideoPlayer {
 
 	/**
 	 * The Resource object for VMDs that are read out of a resource bundle
-	 * instead of being streamed from the filesystem.
+	 * instead of being streamed from the filesystem. The resource is owned by
+	 * ResourceManager.
 	 */
 	Resource *_bundledVmd;
 
@@ -399,12 +400,13 @@ class VMDPlayer : public VideoPlayer {
 
 private:
 	/**
-	 * The plane where the VMD will be drawn.
+	 * The plane where the VMD will be drawn. The plane is owned by GfxFrameout.
 	 */
 	Plane *_plane;
 
 	/**
-	 * The screen item representing the VMD surface.
+	 * The screen item representing the VMD surface. The screen item is owned by
+	 * GfxFrameout.
 	 */
 	ScreenItem *_screenItem;
 
@@ -538,7 +540,7 @@ class VMDPlayer : public VideoPlayer {
 
 	/**
 	 * An optional plane that will be used to black out areas of the screen
-	 * outside of the VMD surface.
+	 * outside of the VMD surface. The plane is owned by GfxFrameout.
 	 */
 	Plane *_blackoutPlane;
 
@@ -675,6 +677,7 @@ class DuckPlayer : public VideoPlayer {
 private:
 	/**
 	 * An empty plane drawn behind the video when the doFrameOut flag is true.
+	 * The plane is owned by GfxFrameout.
 	 */
 	Plane *_plane;