Fix various bugs in NES NTSC filter and PPU palettes

Core/NES/BaseNesPpu.h

@@ -16,6 +16,7 @@ class BaseNesPpu : public INesMemoryHandler, public ISerializable
 {
 protected:
 	uint64_t _masterClock = 0;
+	uint64_t _masterClockFrameStart = 0;
 	uint32_t _cycle = 0;
 	int16_t _scanline = 0;
 	bool _emulatorBgEnabled = false;

Core/NES/BisqwitNtscFilter.cpp

@@ -66,7 +66,7 @@ BisqwitNtscFilter::BisqwitNtscFilter(Emulator* emu) : BaseVideoFilter(emu)
 			int scale = 8 / _resDivider;
 			outputBuffer += frameInfo.Width * ((120 - GetOverscan().Top) * scale);
 
-			DecodeFrame(120, 239 - GetOverscan().Bottom, _ppuOutputBuffer, outputBuffer, (GetVideoPhase() * 4) + 327360);
+			DecodeFrame(120, 239 - GetOverscan().Bottom, _ppuOutputBuffer, outputBuffer, GetVideoPhaseOffset() + 120 * 341 * _signalsPerPixel);
 
 			_workDone = true;
 		}
@@ -90,7 +90,7 @@ void BisqwitNtscFilter::ApplyFilter(uint16_t *ppuOutputBuffer)
 
 	_workDone = false;
 	_waitWork.Signal();
-	DecodeFrame(GetOverscan().Top, 120, ppuOutputBuffer, GetOutputBuffer(), (GetVideoPhase() * 4) + GetOverscan().Top*341*8);
+	DecodeFrame(GetOverscan().Top, 120, ppuOutputBuffer, GetOutputBuffer(), GetVideoPhaseOffset() + GetOverscan().Top * 341 * _signalsPerPixel);
 	while(!_workDone) {}
 }
 
@@ -140,14 +140,28 @@ void BisqwitNtscFilter::OnBeforeApplyFilter()
 
 	_y = contrast / _yWidth;
 
-	_ir = (int)(contrast * 1.994681e-6 * saturation / _iWidth);
-	_qr = (int)(contrast * 9.915742e-7 * saturation / _qWidth);
+	// https://forums.nesdev.org/viewtopic.php?p=172817#p172817
+	// Bisqwit originally intended to match a certain palette using different
+	// display primaries ("FCC D65"). This resulted in colors that looked off.
+	// The modification here is to more closely match Bisqwit's own palette
+	// generator, which uses a more standard RGB-YIQ matrix.
+	// at hue 0, saturation 1.0, contrast 1.0, brightness 1.0, gamma 2.2
+	// https://bisqwit.iki.fi/utils/nespalette.php
 
-	_ig = (int)(contrast * 9.151351e-8 * saturation / _iWidth);
-	_qg = (int)(contrast * -6.334805e-7 * saturation / _qWidth);
+	// (<contrast at 0> * <saturation at 0>) / (<_iWidth or _qWidth at 0> / 2000)
+	double saturationFactor = (167941.0 * 144044.0) / (12.0 * 2000.0);
 
-	_ib = (int)(contrast * -1.012984e-6 * saturation / _iWidth);
-	_qb = (int)(contrast * 1.667217e-6 * saturation / _qWidth);
+	// IQ coefficients are derived from the NTSC base matrix of luminance and color-difference
+	// with color reduction factors and an additional 33 degree rotation of each respective component.
+	// https://www.nesdev.org/wiki/NTSC_video#Converting_YUV_to_signal_RGB
+	_ir = (int)(contrast * ( 0.956084 / saturationFactor) * saturation / _iWidth);
+	_qr = (int)(contrast * ( 0.620888 / saturationFactor) * saturation / _qWidth);
+
+	_ig = (int)(contrast * (-0.272281 / saturationFactor) * saturation / _iWidth);
+	_qg = (int)(contrast * (-0.646901 / saturationFactor) * saturation / _qWidth);
+
+	_ib = (int)(contrast * (-1.105617 / saturationFactor) * saturation / _iWidth);
+	_qb = (int)(contrast * ( 1.702501 / saturationFactor) * saturation / _qWidth);
 }
 
 void BisqwitNtscFilter::RecursiveBlend(int iterationCount, uint64_t *output, uint64_t *currentLine, uint64_t *nextLine, int pixelsPerCycle, bool verticalBlend)

Core/NES/NesDefaultVideoFilter.cpp

@@ -42,31 +42,45 @@ NesDefaultVideoFilter::NesDefaultVideoFilter(Emulator* emu) : BaseVideoFilter(em
 	InitLookupTable();
 }
 
-void NesDefaultVideoFilter::GenerateFullColorPalette(uint32_t paletteBuffer[512])
+void NesDefaultVideoFilter::GenerateFullColorPalette(uint32_t paletteBuffer[512], PpuModel model)
 {
 	for(int i = 0; i < 64; i++) {
 		for(int j = 1; j < 8; j++) {
 			double redColor = (uint8_t)(paletteBuffer[i] >> 16);
 			double greenColor = (uint8_t)(paletteBuffer[i] >> 8);
 			double blueColor = (uint8_t)paletteBuffer[i];
-			if((i & 0x0F) <= 0x0D) {
-				//Emphasis doesn't affect columns $xE and $xF
-				if(j & 0x01) {
-					//Intensify red
-					greenColor *= 0.84;
-					blueColor *= 0.84;
-				}
-				if(j & 0x02) {
-					//Intensify green
-					redColor *= 0.84;
-					blueColor *= 0.84;
-				}
-				if(j & 0x04) {
-					//Intensify blue
-					redColor *= 0.84;
-					greenColor *= 0.84;
+			if(model == PpuModel::Ppu2C02) {
+				if((i & 0x0F) <= 0x0D) {
+					//Emphasis doesn't affect columns $xE and $xF
+					if(j & 0x01) {
+						//Intensify red
+						greenColor *= 0.84;
+						blueColor *= 0.84;
+					}
+					if(j & 0x02) {
+						//Intensify green
+						redColor *= 0.84;
+						blueColor *= 0.84;
+					}
+					if(j & 0x04) {
+						//Intensify blue
+						redColor *= 0.84;
+						greenColor *= 0.84;
+					}
 				}
 			}
+			else {
+				// RGB PPUs do emphasis in a different way
+				//Intensify red
+				if(j & 0x01)
+					redColor = 0xFF;
+				//Intensify green
+				if(j & 0x02)
+					greenColor = 0xFF;
+				//Intensify blue
+				if(j & 0x04)
+					blueColor = 0xFF;
+			}
 
 			uint8_t r = (uint8_t)(redColor > 255 ? 255 : redColor);
 			uint8_t g = (uint8_t)(greenColor > 255 ? 255 : greenColor);
@@ -98,7 +112,7 @@ void NesDefaultVideoFilter::GetFullPalette(uint32_t palette[512], NesConfig& nes
 			}
 		} else {
 			memcpy(palette, _ppuPaletteArgb[(int)model], sizeof(_ppuPaletteArgb[(int)_ppuModel]));
-			GenerateFullColorPalette(palette);
+			GenerateFullColorPalette(palette, model);
 		}
 	}
 }

Core/NES/NesDefaultVideoFilter.h

@@ -25,7 +25,7 @@ class NesDefaultVideoFilter : public BaseVideoFilter
 
 	static void ApplyPalBorder(uint16_t* ppuOutputBuffer);
 
-	static void GenerateFullColorPalette(uint32_t paletteBuffer[512]);
+	static void GenerateFullColorPalette(uint32_t paletteBuffer[512], PpuModel model = PpuModel::Ppu2C02);
 	static void GetFullPalette(uint32_t palette[512], NesConfig& nesCfg, PpuModel model);
 
 	static uint32_t GetDefaultPixelBrightness(uint16_t colorIndex, PpuModel model);

Core/NES/NesNtscFilter.cpp

@@ -79,7 +79,7 @@ void NesNtscFilter::ApplyFilter(uint16_t *ppuOutputBuffer)
 		NesDefaultVideoFilter::ApplyPalBorder(ppuOutputBuffer);
 	}
 
-	nes_ntsc_blit(&_ntscData, ppuOutputBuffer, _baseFrameInfo.Width, GetVideoPhase(), _baseFrameInfo.Width, _baseFrameInfo.Height, _ntscBuffer, NES_NTSC_OUT_WIDTH(_baseFrameInfo.Width) * 4);
+	nes_ntsc_blit(&_ntscData, ppuOutputBuffer, _baseFrameInfo.Width, (GetVideoPhaseOffset() / 4), _baseFrameInfo.Width, _baseFrameInfo.Height, _ntscBuffer, NES_NTSC_OUT_WIDTH(_baseFrameInfo.Width) * 4);
 
 	for(uint32_t i = 0; i < frameInfo.Height; i+=2) {
 		memcpy(GetOutputBuffer()+i*frameInfo.Width, _ntscBuffer + yOffset + xOffset + (i/2)*baseWidth, frameInfo.Width * sizeof(uint32_t));

Core/NES/NesPpu.cpp

@@ -33,6 +33,7 @@ template<class T> NesPpu<T>::NesPpu(NesConsole* console)
 	_emu = console->GetEmulator();
 	_mapper = console->GetMapper();
 	_masterClock = 0;
+	_masterClockFrameStart = 0;
 	_masterClockDivider = 4;
 	_settings = _emu->GetSettings();
 
@@ -78,6 +79,7 @@ template<class T> NesPpu<T>::NesPpu(NesConsole* console)
 template<class T> void NesPpu<T>::Reset(bool softReset)
 {
 	_masterClock = 0;
+	_masterClockFrameStart = 0;
 
 	//Reset OAM decay timestamps regardless of the reset PPU option
 	memset(_oamDecayCycles, 0, sizeof(_oamDecayCycles));
@@ -868,6 +870,10 @@ template<class T> void NesPpu<T>::ProcessScanlineImpl()
 		}
 
 		if(_scanline >= 0) {
+			if(_scanline == 0 && _cycle == 1) {
+				// get the master clock at the first dot
+				_masterClockFrameStart = _masterClock;
+			}
 			((T*)this)->DrawPixel();
 			ShiftTileRegisters();
 
@@ -1109,15 +1115,18 @@ template<class T> void NesPpu<T>::SendFrame()
 		_emu->GetNotificationManager()->SendNotification(ConsoleNotificationType::PpuFrameDone, _currentOutputBuffer);
 	}
 
-	//Get phase at the start of the current frame (341*241 cycles ago)
-	uint32_t videoPhase = ((_masterClock / _masterClockDivider) - 82181) % 3;
+	//Get chroma phase offset at the start of the current frame
+	//In units of color generator clocks
+	//https://www.nesdev.org/wiki/NTSC_video#Color_Artifacts
+	uint32_t videoPhaseOffset = (_masterClockFrameStart % 6) * 2;
+
 	NesConfig& cfg = _console->GetNesConfig();
 	if(_region != ConsoleRegion::Ntsc || cfg.PpuExtraScanlinesAfterNmi != 0 || cfg.PpuExtraScanlinesBeforeNmi != 0) {
 		//Force 2-phase pattern for PAL or when overclocking is used
-		videoPhase = _frameCount & 0x01;
+		videoPhaseOffset = (_frameCount & 0x01) * 4;
 	}
 
-	RenderedFrame frame(_currentOutputBuffer, NesConstants::ScreenWidth, NesConstants::ScreenHeight, 1.0, _frameCount, _console->GetControlManager()->GetPortStates(), videoPhase);
+	RenderedFrame frame(_currentOutputBuffer, NesConstants::ScreenWidth, NesConstants::ScreenHeight, 1.0, _frameCount, _console->GetControlManager()->GetPortStates(), videoPhaseOffset);
 	frame.Data = frameData; //HD packs
 
 	if(_console->GetVsMainConsole() || _console->GetVsSubConsole()) {
@@ -1453,7 +1462,7 @@ template<class T> void NesPpu<T>::Serialize(Serializer& s)
 	SV(_mask.IntensifyBlue); SV(_paletteRamMask); SV(_intensifyColorBits); SV(_statusFlags.SpriteOverflow); SV(_statusFlags.Sprite0Hit); SV(_statusFlags.VerticalBlank); SV(_scanline);
 	SV(_cycle); SV(_frameCount); SV(_memoryReadBuffer); SV(_region);
 
-	SV(_ppuBusAddress); SV(_masterClock);
+	SV(_ppuBusAddress); SV(_masterClock); SV(_masterClockFrameStart);
 
 	if(s.GetFormat() != SerializeFormat::Map) {
 		//Hide these entries from the Lua API

Core/Shared/RenderedFrame.h

@@ -11,7 +11,7 @@ struct RenderedFrame
 	uint32_t Height = 240;
 	double Scale = 1.0;
 	uint32_t FrameNumber = 0;
-	uint32_t VideoPhase = 0;
+	uint32_t VideoPhaseOffset = 0;
 	vector<ControllerData> InputData;
 
 	RenderedFrame()
@@ -27,14 +27,14 @@ struct RenderedFrame
 		InputData({})
 	{}
 
-	RenderedFrame(void* buffer, uint32_t width, uint32_t height, double scale, uint32_t frameNumber, vector<ControllerData> inputData, uint32_t videoPhase = 0) :
+	RenderedFrame(void* buffer, uint32_t width, uint32_t height, double scale, uint32_t frameNumber, vector<ControllerData> inputData, uint32_t videoPhaseOffset = 0) :
 		FrameBuffer(buffer),
 		Data(nullptr),
 		Width(width),
 		Height(height),
 		Scale(scale),
 		FrameNumber(frameNumber),
-		VideoPhase(videoPhase),
+		VideoPhaseOffset(videoPhaseOffset),
 		InputData(inputData)
 	{}
 };

Core/Shared/Video/BaseVideoFilter.cpp

@@ -71,9 +71,9 @@ bool BaseVideoFilter::IsOddFrame()
 	return _isOddFrame;
 }
 
-uint32_t BaseVideoFilter::GetVideoPhase()
+uint32_t BaseVideoFilter::GetVideoPhaseOffset()
 {
-	return _videoPhase;
+	return _videoPhaseOffset;
 }
 
 uint32_t BaseVideoFilter::GetBufferSize()
@@ -92,12 +92,12 @@ FrameInfo BaseVideoFilter::GetFrameInfo(uint16_t* ppuOutputBuffer, bool enableOv
 	return frameInfo;
 }
 
-FrameInfo BaseVideoFilter::SendFrame(uint16_t *ppuOutputBuffer, uint32_t frameNumber, uint32_t videoPhase, void* frameData, bool enableOverscan)
+FrameInfo BaseVideoFilter::SendFrame(uint16_t *ppuOutputBuffer, uint32_t frameNumber, uint32_t videoPhaseOffset, void* frameData, bool enableOverscan)
 {
 	auto lock = _frameLock.AcquireSafe();
 	_overscan = enableOverscan ? _emu->GetSettings()->GetOverscan() : OverscanDimensions{};
 	_isOddFrame = frameNumber % 2;
-	_videoPhase = videoPhase;
+	_videoPhaseOffset = videoPhaseOffset;
 	_frameData = frameData;
 	_ppuOutputBuffer = ppuOutputBuffer;
 	OnBeforeApplyFilter();

Core/Shared/Video/BaseVideoFilter.h

@@ -14,7 +14,7 @@ class BaseVideoFilter
 	SimpleLock _frameLock;
 	OverscanDimensions _overscan = {};
 	bool _isOddFrame = false;
-	uint32_t _videoPhase = 0;
+	uint32_t _videoPhaseOffset = 0;
 
 	void UpdateBufferSize();
 
@@ -33,7 +33,7 @@ class BaseVideoFilter
 	virtual void ApplyFilter(uint16_t *ppuOutputBuffer) = 0;
 	virtual void OnBeforeApplyFilter();
 	bool IsOddFrame();
-	uint32_t GetVideoPhase();
+	uint32_t GetVideoPhaseOffset();
 	uint32_t GetBufferSize();
 
 	template<typename T> bool NtscFilterOptionsChanged(T& ntscSetup);
@@ -47,7 +47,7 @@ class BaseVideoFilter
 	virtual ~BaseVideoFilter();
 
 	uint32_t* GetOutputBuffer();
-	FrameInfo SendFrame(uint16_t *ppuOutputBuffer, uint32_t frameNumber, uint32_t videoPhase, void* frameData, bool enableOverscan = true);
+	FrameInfo SendFrame(uint16_t *ppuOutputBuffer, uint32_t frameNumber, uint32_t videoPhaseOffset, void* frameData, bool enableOverscan = true);
 	void TakeScreenshot(string romName, VideoFilterType filterType);
 	void TakeScreenshot(VideoFilterType filterType, string filename, std::stringstream *stream = nullptr);
 

Core/Shared/Video/VideoDecoder.cpp

@@ -105,7 +105,7 @@ void VideoDecoder::DecodeFrame(bool forRewind)
 	}
 
 	_videoFilter->SetBaseFrameInfo(_baseFrameSize);
-	FrameInfo frameSize = _videoFilter->SendFrame((uint16_t*)_frame.FrameBuffer, _frame.FrameNumber, _frame.VideoPhase, _frame.Data);
+	FrameInfo frameSize = _videoFilter->SendFrame((uint16_t*)_frame.FrameBuffer, _frame.FrameNumber, _frame.VideoPhaseOffset, _frame.Data);
 
 	uint32_t* outputBuffer = _videoFilter->GetOutputBuffer();