SCI32: Improve playback quality of SEQ videos

3 files changed, 147 insertions, 146 deletions

diff --git a/engines/sci/detection_tables.h b/engines/sci/detection_tables.h
index 9378f2b1ec..4230a3bb0f 100644
--- a/engines/sci/detection_tables.h
+++ b/engines/sci/detection_tables.h
@@ -724,10 +724,7 @@ static const struct ADGameDescription SciGameDescriptions[] = {
 #ifdef ENABLE_SCI32
 #define GUIO_GK1_FLOPPY GUIO2(GUIO_NOSPEECH, \
                               GAMEOPTION_ORIGINAL_SAVELOAD)
-#define GUIO_GK1_CD_DOS GUIO3(GUIO_LINKSPEECHTOSFX, \
-                              GAMEOPTION_ORIGINAL_SAVELOAD, \
-                              GAMEOPTION_HIGH_RESOLUTION_GRAPHICS)
-#define GUIO_GK1_CD_WIN GUIO4(GUIO_LINKSPEECHTOSFX, \
+#define GUIO_GK1_CD     GUIO4(GUIO_LINKSPEECHTOSFX, \
                               GAMEOPTION_ORIGINAL_SAVELOAD, \
                               GAMEOPTION_HIGH_RESOLUTION_GRAPHICS, \
                               GAMEOPTION_HQ_VIDEO)
@@ -779,7 +776,7 @@ static const struct ADGameDescription SciGameDescriptions[] = {
 		{"resource.map", 0, "372d059f75856afa6d73dd84cbb8913d", 10996},
 		{"resource.000", 0, "69b7516962510f780d38519cc15fcc7c", 12581736},
 		AD_LISTEND},
-		Common::EN_ANY, Common::kPlatformDOS, ADGF_CD | ADGF_TESTING, GUIO_GK1_CD_DOS },
+		Common::EN_ANY, Common::kPlatformDOS, ADGF_CD | ADGF_TESTING, GUIO_GK1_CD },
 
 	// Gabriel Knight - English Windows CD (from jvprat)
 	// Executable scanning reports "2.000.000", VERSION file reports "01.100.000"
@@ -787,7 +784,7 @@ static const struct ADGameDescription SciGameDescriptions[] = {
 		{"resource.map", 0, "372d059f75856afa6d73dd84cbb8913d", 10996},
 		{"resource.000", 0, "69b7516962510f780d38519cc15fcc7c", 12581736},
 		AD_LISTEND},
-		Common::EN_ANY, Common::kPlatformWindows, ADGF_CD | ADGF_TESTING, GUIO_GK1_CD_WIN },
+		Common::EN_ANY, Common::kPlatformWindows, ADGF_CD | ADGF_TESTING, GUIO_GK1_CD },
 
 	// Gabriel Knight - German DOS CD (from Tobis87)
 	// SCI interpreter version 2.000.000
@@ -795,7 +792,7 @@ static const struct ADGameDescription SciGameDescriptions[] = {
 		{"resource.map", 0, "a7d3e55114c65647310373cb390815ba", 11392},
 		{"resource.000", 0, "091cf08910780feabc56f8551b09cb36", 13400497},
 		AD_LISTEND},
-		Common::DE_DEU, Common::kPlatformDOS, ADGF_CD | ADGF_TESTING, GUIO_GK1_CD_DOS },
+		Common::DE_DEU, Common::kPlatformDOS, ADGF_CD | ADGF_TESTING, GUIO_GK1_CD },
 
 	// Gabriel Knight - Spanish DOS CD (from jvprat)
 	// Executable scanning reports "2.000.000", VERSION file reports "1.000.000, April 13, 1995"
@@ -803,7 +800,7 @@ static const struct ADGameDescription SciGameDescriptions[] = {
 		{"resource.map", 0, "7cb6e9bba15b544ec7a635c45bde9953", 11404},
 		{"resource.000", 0, "091cf08910780feabc56f8551b09cb36", 13381599},
 		AD_LISTEND},
-		Common::ES_ESP, Common::kPlatformDOS, ADGF_CD | ADGF_TESTING, GUIO_GK1_CD_DOS },
+		Common::ES_ESP, Common::kPlatformDOS, ADGF_CD | ADGF_TESTING, GUIO_GK1_CD },
 
 	// Gabriel Knight - French DOS CD (from Hkz)
 	// VERSION file reports "1.000.000, May 3, 1994"
@@ -811,7 +808,7 @@ static const struct ADGameDescription SciGameDescriptions[] = {
 		{"resource.map", 0, "55f909ba93a2515042a08d8a2da8414e", 11392},
 		{"resource.000", 0, "091cf08910780feabc56f8551b09cb36", 13325145},
 		AD_LISTEND},
-		Common::FR_FRA, Common::kPlatformDOS, ADGF_CD | ADGF_TESTING, GUIO_GK1_CD_DOS },
+		Common::FR_FRA, Common::kPlatformDOS, ADGF_CD | ADGF_TESTING, GUIO_GK1_CD },
 
 	// Gabriel Knight - Spanish Windows CD (from jvprat)
 	// Executable scanning reports "2.000.000", VERSION file reports "1.000.000, April 13, 1995"
@@ -819,7 +816,7 @@ static const struct ADGameDescription SciGameDescriptions[] = {
 		{"resource.map", 0, "7cb6e9bba15b544ec7a635c45bde9953", 11404},
 		{"resource.000", 0, "091cf08910780feabc56f8551b09cb36", 13381599},
 		AD_LISTEND},
-		Common::ES_ESP, Common::kPlatformWindows, ADGF_CD | ADGF_TESTING, GUIO_GK1_CD_WIN },
+		Common::ES_ESP, Common::kPlatformWindows, ADGF_CD | ADGF_TESTING, GUIO_GK1_CD },
 
 	// Gabriel Knight - English Macintosh (Floppy!)
 	// This version is hi-res ONLY, so it should NOT get GAMEOPTION_HIGH_RESOLUTION_GRAPHICS
@@ -834,8 +831,7 @@ static const struct ADGameDescription SciGameDescriptions[] = {
 		Common::EN_ANY, Common::kPlatformMacintosh, ADGF_MACRESFORK | ADGF_UNSTABLE, GUIO_GK1_MAC },
 
 #undef GUIO_GK1_FLOPPY
-#undef GUIO_GK1_CD_DOS
-#undef GUIO_GK1_CD_WIN
+#undef GUIO_GK1_CD
 #undef GUIO_GK1_MAC
 
 #define GUIO_GK2_DEMO GUIO8(GUIO_NOSUBTITLES, \
@@ -3915,8 +3911,9 @@ static const struct ADGameDescription SciGameDescriptions[] = {
 
 #define GUIO_QFG4_FLOPPY GUIO2(GUIO_NOSPEECH, \
                                GAMEOPTION_ORIGINAL_SAVELOAD)
-#define GUIO_QFG4_CD     GUIO2(GUIO_LINKSPEECHTOSFX, \
-                               GAMEOPTION_ORIGINAL_SAVELOAD)
+#define GUIO_QFG4_CD     GUIO3(GUIO_LINKSPEECHTOSFX, \
+                               GAMEOPTION_ORIGINAL_SAVELOAD, \
+                               GAMEOPTION_HQ_VIDEO)
 
 	// Quest for Glory 4 1.1 Floppy - English DOS (supplied by markcool in bug report #2723852)
 	// SCI interpreter version 2.000.000 (a guess?)
diff --git a/engines/sci/graphics/video32.cpp b/engines/sci/graphics/video32.cpp
index b05b9e6868..7d2daf787a 100644
--- a/engines/sci/graphics/video32.cpp
+++ b/engines/sci/graphics/video32.cpp
@@ -75,55 +75,138 @@ static bool flushEvents(EventManager *eventMan) {
 	return false;
 }
 
+static void directWriteToSystem(Video::VideoDecoder *decoder, const Common::Rect &drawRect, const bool setSystemPalette, const Graphics::Surface *nextFrame = nullptr) {
+
+	// VMDPlayer needs to decode the frame early so it can submit palette
+	// updates; calling decodeNextFrame again loses frames
+	if (!nextFrame) {
+		nextFrame = decoder->decodeNextFrame();
+	}
+	assert(nextFrame);
+
+	if (setSystemPalette &&
+		g_system->getScreenFormat().bytesPerPixel == 1 &&
+		decoder->hasDirtyPalette()) {
+
+		const uint8 *palette = decoder->getPalette();
+		assert(palette);
+		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
+		for (int color = 0; color < 256; ++color) {
+			if (palette[0] == 0 && palette[1] == 0 && palette[2] == 0) {
+				g_system->fillScreen(color);
+				break;
+			}
+			palette += 3;
+		}
+	}
+
+	bool freeConvertedFrame;
+	Graphics::Surface *convertedFrame;
+	// Avoid creating a duplicate copy of the surface when it is not necessary
+	if (decoder->getPixelFormat() == g_system->getScreenFormat()) {
+		freeConvertedFrame = false;
+		convertedFrame = const_cast<Graphics::Surface *>(nextFrame);
+	} else {
+		freeConvertedFrame = true;
+		convertedFrame = nextFrame->convertTo(g_system->getScreenFormat(), decoder->getPalette());
+	}
+	assert(convertedFrame);
+
+	if (decoder->getWidth() != drawRect.width() || decoder->getHeight() != drawRect.height()) {
+		Graphics::Surface *const unscaledFrame(convertedFrame);
+		const Graphics::TransparentSurface tsUnscaledFrame(*unscaledFrame);
+#ifdef USE_RGB_COLOR
+		if (g_system->getScreenFormat().bytesPerPixel != 1) {
+			convertedFrame = tsUnscaledFrame.scaleT<Graphics::FILTER_BILINEAR>(drawRect.width(), drawRect.height());
+		} else {
+#else
+		{
+#endif
+			convertedFrame = tsUnscaledFrame.scaleT<Graphics::FILTER_NEAREST>(drawRect.width(), drawRect.height());
+		}
+		assert(convertedFrame);
+		if (freeConvertedFrame) {
+			unscaledFrame->free();
+			delete unscaledFrame;
+		}
+		freeConvertedFrame = true;
+	}
+
+	g_system->copyRectToScreen(convertedFrame->getPixels(), convertedFrame->pitch, drawRect.left, drawRect.top, convertedFrame->w, convertedFrame->h);
+	g_sci->_gfxFrameout->updateScreen();
+	if (freeConvertedFrame) {
+		convertedFrame->free();
+		delete convertedFrame;
+	}
+}
+
 #pragma mark SEQPlayer
 
 SEQPlayer::SEQPlayer(SegManager *segMan, EventManager *eventMan) :
 	_segMan(segMan),
 	_eventMan(eventMan),
-	_decoder(nullptr),
-	_plane(nullptr),
-	_screenItem(nullptr) {}
+	_decoder(nullptr) {}
 
 void SEQPlayer::play(const Common::String &fileName, const int16 numTicks, const int16 x, const int16 y) {
-	delete _decoder;
+
+	close();
+
 	_decoder = new SEQDecoder(numTicks);
 	if (!_decoder->loadFile(fileName)) {
 		warning("[SEQPlayer::play]: Failed to load %s", fileName.c_str());
+		delete _decoder;
 		return;
 	}
 
-	// NOTE: In the original engine, video was output directly to the hardware,
-	// bypassing the game's rendering engine. Instead of doing this, we use a
-	// mechanism that is very similar to that used by the VMD player, which
-	// allows the SEQ to be drawn into a bitmap ScreenItem and displayed using
-	// the normal graphics system.
-	reg_t bitmapId;
-	SciBitmap &bitmap = *_segMan->allocateBitmap(&bitmapId, _decoder->getWidth(), _decoder->getHeight(), kDefaultSkipColor, 0, 0, kLowResX, kLowResY, 0, false, false);
-	bitmap.getBuffer().fillRect(Common::Rect(_decoder->getWidth(), _decoder->getHeight()), 0);
-
-	CelInfo32 celInfo;
-	celInfo.type = kCelTypeMem;
-	celInfo.bitmap = bitmapId;
-
-	_plane = new Plane(Common::Rect(kLowResX, kLowResY), kPlanePicColored);
-	g_sci->_gfxFrameout->addPlane(*_plane);
-
-	// Normally we would use the x, y coordinates passed into the play function
-	// to position the screen item, but because the video frame bitmap is
-	// drawn in low-resolution coordinates, it gets automatically scaled up by
-	// the engine (pixel doubling with aspect ratio correction). As a result,
-	// the animation does not need the extra offsets from the game in order to
-	// be correctly positioned in the middle of the window, so we ignore them.
-	_screenItem = new ScreenItem(_plane->_object, celInfo, Common::Point(0, 0), ScaleInfo());
-	g_sci->_gfxFrameout->addScreenItem(*_screenItem);
-	g_sci->_gfxFrameout->frameOut(true);
+	const int16 scriptWidth = g_sci->_gfxFrameout->getCurrentBuffer().scriptWidth;
+	const int16 scriptHeight = g_sci->_gfxFrameout->getCurrentBuffer().scriptHeight;
+	const int16 screenWidth = g_sci->_gfxFrameout->getCurrentBuffer().screenWidth;
+	const int16 screenHeight = g_sci->_gfxFrameout->getCurrentBuffer().screenHeight;
+
+	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
+	// intended to play in the center of the screen, we just recalculate the
+	// origin here.
+	_drawRect.left = (screenWidth - scaledWidth) / 2;
+	_drawRect.top = (screenHeight - scaledHeight) / 2;
+	_drawRect.setWidth(scaledWidth);
+	_drawRect.setHeight(scaledHeight);
+
+#ifdef USE_RGB_COLOR
+	// Optimize rendering performance for unscaled videos, and allow
+	// better-than-NN interpolation for videos that are scaled
+	if (ConfMan.getBool("enable_hq_video") &&
+		(_decoder->getWidth() != scaledWidth || _decoder->getHeight() != scaledHeight)) {
+		// TODO: Search for and use the best supported format (which may be
+		// lower than 32bpp) once the scaling code in Graphics supports
+		// 16bpp/24bpp, and once the SDL backend can correctly communicate
+		// supported pixel formats above whatever format is currently used by
+		// _hwsurface. Right now, this will just crash ScummVM if the backend
+		// does not support a 32bpp pixel format, which sucks since this code
+		// really ought to be able to fall back to NN scaling for games with
+		// 256-color videos.
+		const Graphics::PixelFormat format = Graphics::createPixelFormat<8888>();
+		g_sci->_gfxFrameout->setPixelFormat(format);
+	}
+#endif
+
 	_decoder->start();
 
 	while (!g_engine->shouldQuit() && !_decoder->endOfVideo()) {
 		g_sci->sleep(_decoder->getTimeToNextFrame());
-
 		while (_decoder->needsUpdate()) {
-			renderFrame(bitmap);
+			renderFrame();
 		}
 
 		// SSCI did not allow SEQ animations to be bypassed like this
@@ -149,34 +232,24 @@ void SEQPlayer::play(const Common::String &fileName, const int16 numTicks, const
 		}
 	}
 
-	_segMan->freeBitmap(bitmapId);
-	g_sci->_gfxFrameout->deletePlane(*_plane);
-	g_sci->_gfxFrameout->frameOut(true);
-	_screenItem = nullptr;
-	_plane = nullptr;
+	close();
 }
 
-void SEQPlayer::renderFrame(SciBitmap &bitmap) const {
-	const Graphics::Surface *surface = _decoder->decodeNextFrame();
-
-	bitmap.getBuffer().copyRectToSurface(*surface, 0, 0, Common::Rect(surface->w, surface->h));
-
-	const bool dirtyPalette = _decoder->hasDirtyPalette();
-	if (dirtyPalette) {
-		Palette palette;
-		const byte *rawPalette = _decoder->getPalette();
-		for (int i = 0; i < ARRAYSIZE(palette.colors); ++i) {
-			palette.colors[i].r = *rawPalette++;
-			palette.colors[i].g = *rawPalette++;
-			palette.colors[i].b = *rawPalette++;
-			palette.colors[i].used = true;
-		}
+void SEQPlayer::renderFrame() const {
+	directWriteToSystem(_decoder, _drawRect, true);
+}
 
-		g_sci->_gfxPalette32->submit(palette);
+void SEQPlayer::close() {
+#ifdef USE_RGB_COLOR
+	if (g_system->getScreenFormat().bytesPerPixel != 1) {
+		const Graphics::PixelFormat format = Graphics::PixelFormat::createFormatCLUT8();
+		g_sci->_gfxFrameout->setPixelFormat(format);
 	}
+#endif
 
-	g_sci->_gfxFrameout->updateScreenItem(*_screenItem);
-	g_sci->_gfxFrameout->frameOut(true);
+	g_system->fillScreen(0);
+	delete _decoder;
+	_decoder = nullptr;
 }
 
 #pragma mark -
@@ -365,78 +438,8 @@ uint16 AVIPlayer::getDuration() const {
 	return _decoder->getFrameCount();
 }
 
-template <Graphics::TFilteringMode MODE>
-static void writeFrameToSystem(const Graphics::Surface *nextFrame, Video::VideoDecoder *decoder, const Common::Rect &drawRect) {
-	assert(nextFrame);
-
-	bool freeConvertedFrame;
-	Graphics::Surface *convertedFrame;
-	// Avoid creating a duplicate copy of the surface when it is not necessary
-	if (decoder->getPixelFormat() == g_system->getScreenFormat()) {
-		freeConvertedFrame = false;
-		convertedFrame = const_cast<Graphics::Surface *>(nextFrame);
-	} else {
-		freeConvertedFrame = true;
-		convertedFrame = nextFrame->convertTo(g_system->getScreenFormat(), decoder->getPalette());
-	}
-	assert(convertedFrame);
-
-	if (decoder->getWidth() != drawRect.width() || decoder->getHeight() != drawRect.height()) {
-		Graphics::Surface *const unscaledFrame(convertedFrame);
-		const Graphics::TransparentSurface tsUnscaledFrame(*unscaledFrame);
-		convertedFrame = tsUnscaledFrame.scaleT<MODE>(drawRect.width(), drawRect.height());
-		assert(convertedFrame);
-		if (freeConvertedFrame) {
-			unscaledFrame->free();
-			delete unscaledFrame;
-		}
-		freeConvertedFrame = true;
-	}
-
-	g_system->copyRectToScreen(convertedFrame->getPixels(), convertedFrame->pitch, drawRect.left, drawRect.top, convertedFrame->w, convertedFrame->h);
-	g_sci->_gfxFrameout->updateScreen();
-	if (freeConvertedFrame) {
-		convertedFrame->free();
-		delete convertedFrame;
-	}
-}
-
 void AVIPlayer::renderFrame() const {
-	// TODO: Improve efficiency by making changes to common Graphics code that
-	// allow reuse of a single conversion surface for all frames
-
-	const Graphics::Surface *nextFrame = _decoder->decodeNextFrame();
-	assert(nextFrame);
-
-	if (g_system->getScreenFormat().bytesPerPixel == 1 && _decoder->hasDirtyPalette()) {
-		const uint8 *palette = _decoder->getPalette();
-		assert(palette);
-		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
-		for (int color = 0; color < 256; ++color) {
-			if (palette[0] == 0 && palette[1] == 0 && palette[2] == 0) {
-				g_system->fillScreen(color);
-				break;
-			}
-			palette += 3;
-		}
-	}
-
-#ifdef USE_RGB_COLOR
-	if (g_system->getScreenFormat().bytesPerPixel != 1) {
-		writeFrameToSystem<Graphics::FILTER_BILINEAR>(nextFrame, _decoder, _drawRect);
-	} else {
-#else
-	{
-#endif
-		writeFrameToSystem<Graphics::FILTER_NEAREST>(nextFrame, _decoder, _drawRect);
-	}
+	directWriteToSystem(_decoder, _drawRect, true);
 }
 
 AVIPlayer::EventFlags AVIPlayer::playUntilEvent(EventFlags flags) {
@@ -821,7 +824,7 @@ void VMDPlayer::renderOverlay() const {
 			redrawGameScreen();
 		}
 
-		writeFrameToSystem<Graphics::FILTER_BILINEAR>(nextFrame, _decoder, _drawRect);
+		directWriteToSystem(_decoder, _drawRect, false, nextFrame);
 	} else {
 #else
 	{
diff --git a/engines/sci/graphics/video32.h b/engines/sci/graphics/video32.h
index 0f63996040..474851cdf5 100644
--- a/engines/sci/graphics/video32.h
+++ b/engines/sci/graphics/video32.h
@@ -68,19 +68,20 @@ private:
 	SEQDecoder *_decoder;
 
 	/**
-	 * The plane where the SEQ will be drawn.
+	 * Renders a single frame of video.
 	 */
-	Plane *_plane;
+	void renderFrame() const;
 
 	/**
-	 * The screen item representing the SEQ surface.
+	 * Stops playback and closes the currently open SEQ stream.
 	 */
-	ScreenItem *_screenItem;
+	void close();
 
 	/**
-	 * Renders a single frame of video.
+	 * The rectangle where the video will be drawn,
+	 * in screen coordinates.
 	 */
-	void renderFrame(SciBitmap &bitmap) const;
+	Common::Rect _drawRect;
 };
 
 #pragma mark -