From 5026de9653e50a04c45a4caa4913c9e06750f4e4 Mon Sep 17 00:00:00 2001 From: Thomas Harte Date: Fri, 27 Dec 2019 22:47:34 -0500 Subject: [PATCH] Rejigs the video stream to ensure shifter really is continuous. ... and definitively to avoid potential buffer overruns. Or, at least, to have a mechanism in place definitively to avoid them. Which will be tested and debugged as necessary. Also simplifies the colour burst and border/pixels selection logic. --- Machines/Atari/ST/Video.cpp | 357 ++++++++++++++++++++---------------- Machines/Atari/ST/Video.hpp | 50 +++-- 2 files changed, 235 insertions(+), 172 deletions(-) diff --git a/Machines/Atari/ST/Video.cpp b/Machines/Atari/ST/Video.cpp index 5ee0d5a2b..8ed5fa99d 100644 --- a/Machines/Atari/ST/Video.cpp +++ b/Machines/Atari/ST/Video.cpp @@ -201,11 +201,11 @@ void Video::advance(HalfCycles duration) { // inside the shifter on the temporary register values. if(horizontal_.sync || vertical_.sync) { - video_stream_.output_sync(run_length); + video_stream_.output(run_length, VideoStream::OutputMode::Sync); } else if(horizontal_.blank || vertical_.blank) { - video_stream_.output_blank(run_length); + video_stream_.output(run_length, VideoStream::OutputMode::Blank); } else if(!load_) { - video_stream_.output_border(run_length, output_bpp_); + video_stream_.output(run_length, VideoStream::OutputMode::Pixels); } else { const int since_load = x_ - load_base_; @@ -222,12 +222,12 @@ void Video::advance(HalfCycles duration) { // was reloaded by the fetch depends on the FIFO. if(start_column == end_column) { - video_stream_.output_pixels(run_length, output_bpp_); + video_stream_.output(run_length, VideoStream::OutputMode::Pixels); } else { // Continue the current column if partway across. if(since_load&7) { // If at least one column boundary is crossed, complete this column. - video_stream_.output_pixels(8 - (since_load & 7), output_bpp_); + video_stream_.output(8 - (since_load & 7), VideoStream::OutputMode::Pixels); ++start_column; // This starts a new column, so latch a new word. push_latched_data(); } @@ -235,13 +235,13 @@ void Video::advance(HalfCycles duration) { // Run for all columns that have their starts in this time period. int complete_columns = end_column - start_column; while(complete_columns--) { - video_stream_.output_pixels(8, output_bpp_); + video_stream_.output(8, VideoStream::OutputMode::Pixels); push_latched_data(); } // Output the start of the next column, if necessary. if((since_load + run_length) & 7) { - video_stream_.output_pixels((since_load + run_length) & 7, output_bpp_); + video_stream_.output((since_load + run_length) & 7, VideoStream::OutputMode::Pixels); } } } @@ -484,154 +484,172 @@ void Video::update_output_mode() { if(output_bpp_ != old_bpp_) { // "the 71-Hz-switch does something like a shifter-reset." (and some people use a high-low resolutions switch instead) reset_fifo(); + video_stream_.set_bpp(output_bpp_); } } // MARK: - The shifter -void Video::VideoStream::flush_output(OutputMode next_mode) { - switch(output_mode_) { - case OutputMode::Sync: crt_.output_sync(duration_); break; - case OutputMode::Blank: crt_.output_blank(duration_); break; - case OutputMode::ColourBurst: crt_.output_default_colour_burst(duration_); break; - case OutputMode::Border: { -// if(!border_colour_) { -// crt_.output_blank(duration_); -// } else { - uint16_t *const colour_pointer = reinterpret_cast(crt_.begin_data(1)); - if(colour_pointer) *colour_pointer = border_colour_; - crt_.output_level(duration_); -// } - } break; - case OutputMode::Pixels: { - crt_.output_data(duration_, pixel_pointer_); - pixel_buffer_ = nullptr; - pixel_pointer_ = 0; - } break; - } - duration_ = 0; - output_mode_ = next_mode; -} - -void Video::VideoStream::output_colour_burst(int duration) { - // More hackery afoot here; if and when duration_ crosses a threshold of 40, - // output 40 cycles of colour burst and then redirect to blank. - if(output_mode_ != OutputMode::ColourBurst) { - flush_output(OutputMode::ColourBurst); - } - duration_ += duration; - if(duration_ >= 40) { - const int blank_duration = duration_ - 40; - duration_ = 40; - flush_output(OutputMode::Blank); - output_blank(blank_duration); - } -} - -void Video::VideoStream::output_blank(int duration) { - if(output_mode_ != OutputMode::Blank) { - // Bit of a hack: if this is a transition from sync or we're really in - // colour burst, divert into that. - if(output_mode_ == OutputMode::Sync || output_mode_ == OutputMode::ColourBurst) { - output_colour_burst(duration); - return; - } - flush_output(OutputMode::Blank); - } - duration_ += duration; -} - -void Video::VideoStream::output_sync(int duration) { - if(output_mode_ != OutputMode::Sync) { - flush_output(OutputMode::Sync); - } - duration_ += duration; -} - -void Video::VideoStream::output_border(int duration, OutputBpp bpp) { - // If there's still anything in the shifter, redirect this to an output_pixels call. - if(output_shifter_) { - // This doesn't take an opinion on how much of the shifter remains populated; - // it assumes the worst case. - const int pixel_length = std::min(32, duration); - output_pixels(pixel_length, bpp); - duration -= pixel_length; - if(!duration) { - return; - } +void Video::VideoStream::output(int duration, OutputMode mode) { + // If this is a transition from sync to blank, actually transition to colour burst. + if(output_mode_ == OutputMode::Sync && mode == OutputMode::Blank) { + mode = OutputMode::ColourBurst; } - // Flush anything that isn't level output *in the current border colour*. - if(output_mode_ != OutputMode::Border || border_colour_ != palette_[0]) { - flush_output(OutputMode::Border); - border_colour_ = palette_[0]; - } - duration_ += duration; -} + // If this is seeming a transition from blank to colour burst, obey it only if/when + // sufficient colour burst has been output. + if(output_mode_ == OutputMode::Blank && mode == OutputMode::ColourBurst) { + if(duration_ + duration >= 40) { + const int overage = duration + duration_ - 40; + duration_ = 40; -void Video::VideoStream::output_pixels(int duration, OutputBpp bpp) { - // If the shifter is empty and there's no pixel buffer at present, - // redirect this to an output_level call. Otherwise, do a quick - // memset-type fill, since the special case has been detected anyway. - if(!output_shifter_) { - if(!pixel_buffer_) { - output_border(duration, bpp); + generate(overage, OutputMode::ColourBurst, true); } else { - duration_ += duration; - - switch(bpp_) { - case OutputBpp::One: { - const size_t pixels = size_t(duration << 1); - memset(&pixel_buffer_[pixel_pointer_], 0, pixels * sizeof(uint16_t)); - pixel_pointer_ += pixels; - } break; - - default: - case OutputBpp::Four: - assert(!(duration & 1)); - duration >>= 1; - case OutputBpp::Two: { - while(duration--) { - pixel_buffer_[pixel_pointer_] = palette_[0]; - ++pixel_pointer_; - } - } break; - } + mode = OutputMode::ColourBurst; } + } + + // If this is a transition, or if we're doing pixels, output whatever has been accumulated. + if(mode != output_mode_ || output_mode_ == OutputMode::Pixels) { + generate(duration, output_mode_, mode != output_mode_); + } else { + duration_ += duration; + } + + // Accumulate time in the current mode. + output_mode_ = mode; +} + +void Video::VideoStream::generate(int duration, OutputMode mode, bool is_terminal) { + // Three of these are trivial; deal with them upfront. They don't care about the duration of + // whatever is new, just about how much was accumulated prior to now. + if(mode != OutputMode::Pixels) { + switch(mode) { + default: + case OutputMode::Sync: crt_.output_sync(duration_); break; + case OutputMode::Blank: crt_.output_blank(duration_); break; + case OutputMode::ColourBurst: crt_.output_default_colour_burst(duration_); break; + } + + // Reseed duration + duration_ = duration; + + // The shifter should keep running, so throw away the proper amount of content. + shift(duration_); + return; } - // Flush anything that isn't pixel output in the proper bpp; also flush if there's nowhere - // left to put pixels. - if(output_mode_ != OutputMode::Pixels || bpp_ != bpp || pixel_pointer_ >= 320) { - flush_output(OutputMode::Pixels); - bpp_ = bpp; - pixel_buffer_ = reinterpret_cast(crt_.begin_data(320 + 32)); + // If the background colour has changed and border is accumulated, flush it. + if(border_colour_ != palette_[0] && duration_) { + flush_border(); } - duration_ += duration; + border_colour_ = palette_[0]; + // If the shifter is empty, accumulate in duration_ a promise to draw border later. + if(!output_shifter_) { + if(pixel_pointer_) { + flush_pixels(); + } + + duration_ += duration; + + // If this is terminal, we'll need to draw now. But if it isn't, job done. + if(is_terminal) { + flush_border(); + } + + return; + } + + // There's definitely some pixels to convey, but perhaps there's some border first? + if(duration_) { + flush_border(); + } + + // Time to do some pixels! + output_pixels(duration); + + // If was terminal, make sure any transient storage is output. + if(is_terminal) { + flush_pixels(); + } +} + +void Video::VideoStream::flush_border() { + // Output colour 0 for the entirety of duration_ (or black, if this is 1bpp mode). + uint16_t *const colour_pointer = reinterpret_cast(crt_.begin_data(1)); + if(colour_pointer) *colour_pointer = (bpp_ != OutputBpp::One) ? palette_[0] : 0; + crt_.output_level(duration_); + + duration_ = 0; +} + +namespace { +#if TARGET_RT_BIG_ENDIAN + constexpr int upper = 0; +#else + constexpr int upper = 1; +#endif +} + +void Video::VideoStream::shift(int duration) { switch(bpp_) { - case OutputBpp::One: { - int pixels = duration << 1; - if(pixel_buffer_) { - while(pixels--) { + case OutputBpp::One: + output_shifter_ <<= (duration << 1); + break; + case OutputBpp::Two: + while(duration--) { + shifter_halves_[upper] = (shifter_halves_[upper] << 1) & 0xfffefffe; + shifter_halves_[upper] |= (shifter_halves_[upper^1] & 0x80008000) >> 15; + shifter_halves_[upper^1] = (shifter_halves_[upper^1] << 1) & 0xfffefffe; + } + break; + case OutputBpp::Four: + while(duration) { + output_shifter_ = (output_shifter_ << 1) & 0xfffefffefffefffe; + duration -= 2; + } + break; + } +} + +// TODO: turn this into a template on current BPP, perhaps? Would avoid reevaluation of the conditional. +void Video::VideoStream::output_pixels(int duration) { + constexpr int allocation_size = 352; // i.e. 320 plus a spare 32. + + // Convert from duration to pixels. + int pixels = duration; + switch(bpp_) { + case OutputBpp::One: pixels <<= 1; break; + default: break; + case OutputBpp::Four: pixels >>= 1; break; + } + + while(pixels) { + // If no buffer is currently available, attempt to allocate one. + if(!pixel_buffer_) { + pixel_buffer_ = reinterpret_cast(crt_.begin_data(allocation_size, 2)); + + // Stop the loop if no buffer is available. + if(!pixel_buffer_) break; + } + + int pixels_to_draw = std::min(allocation_size - pixel_pointer_, pixels); + pixels -= pixels_to_draw; + + switch(bpp_) { + case OutputBpp::One: + while(pixels_to_draw--) { pixel_buffer_[pixel_pointer_] = ((output_shifter_ >> 63) & 1) * 0xffff; output_shifter_ <<= 1; + ++pixel_pointer_; } - } else { - pixel_pointer_ += size_t(pixels); - output_shifter_ <<= pixels; - } - } break; - case OutputBpp::Two: { - #if TARGET_RT_BIG_ENDIAN - constexpr int upper = 0; - #else - constexpr int upper = 1; - #endif - if(pixel_buffer_) { - while(duration--) { + break; + + case OutputBpp::Two: + while(pixels_to_draw--) { pixel_buffer_[pixel_pointer_] = palette_[ ((output_shifter_ >> 63) & 1) | ((output_shifter_ >> 46) & 2) @@ -645,20 +663,10 @@ void Video::VideoStream::output_pixels(int duration, OutputBpp bpp) { ++pixel_pointer_; } - } else { - pixel_pointer_ += size_t(duration); - while(duration--) { - shifter_halves_[upper] = (shifter_halves_[upper] << 1) & 0xfffefffe; - shifter_halves_[upper] |= (shifter_halves_[upper^1] & 0x80008000) >> 15; - shifter_halves_[upper^1] = (shifter_halves_[upper^1] << 1) & 0xfffefffe; - } - } - } break; - default: - case OutputBpp::Four: - assert(!(duration & 1)); - if(pixel_buffer_) { - while(duration) { + break; + + case OutputBpp::Four: + while(pixels_to_draw--) { pixel_buffer_[pixel_pointer_] = palette_[ ((output_shifter_ >> 63) & 1) | ((output_shifter_ >> 46) & 2) | @@ -666,18 +674,51 @@ void Video::VideoStream::output_pixels(int duration, OutputBpp bpp) { ((output_shifter_ >> 12) & 8) ]; output_shifter_ = (output_shifter_ << 1) & 0xfffefffefffefffe; + ++pixel_pointer_; - duration -= 2; } - } else { - pixel_pointer_ += size_t(duration >> 1); - while(duration) { - output_shifter_ = (output_shifter_ << 1) & 0xfffefffefffefffe; - duration -= 2; - } - } - break; + break; + } + + // Check whether the limit has been reached. + if(pixel_pointer_ == allocation_size) { + flush_pixels(); + } } + + // If duration remains, that implies no buffer was available, so + // just do the corresponding shifting and provide proper timing to the CRT. + if(pixels) { + int leftover_duration = pixels; + switch(bpp_) { + case OutputBpp::One: leftover_duration >>= 1; break; + default: break; + case OutputBpp::Four: leftover_duration <<= 1; break; + } + shift(leftover_duration); + crt_.output_data(leftover_duration); + } + + // Duration will be at most 32, since this will need to be interspersed with load() calls. + // Therefore it's always safe to assume that if a buffer is allocated, then it's big enough, + // provided it is flushed fairly proactively. +} + +void Video::VideoStream::flush_pixels() { + switch(bpp_) { + case OutputBpp::One: crt_.output_data(pixel_pointer_ >> 1, size_t(pixel_pointer_)); break; + default: crt_.output_data(pixel_pointer_); break; + case OutputBpp::Four: crt_.output_data(pixel_pointer_ << 1, size_t(pixel_pointer_)); break; + } + + pixel_pointer_ = 0; + pixel_buffer_ = nullptr; +} + +void Video::VideoStream::set_bpp(OutputBpp bpp) { + // TODO: is flushing like this correct? + output_shifter_ = 0; + bpp_ = bpp; } void Video::VideoStream::load(uint64_t value) { diff --git a/Machines/Atari/ST/Video.hpp b/Machines/Atari/ST/Video.hpp index b436602af..f6345821d 100644 --- a/Machines/Atari/ST/Video.hpp +++ b/Machines/Atari/ST/Video.hpp @@ -166,22 +166,48 @@ class Video { void reset_fifo(); + /*! + Provides a target for control over the output video stream, which is considered to be + a permanently shifting shifter, that you need to reload when appropriate, which can be + overridden by the blank and sync levels. + + This stream will automatically insert a colour burst. + */ class VideoStream { public: VideoStream(Outputs::CRT::CRT &crt, uint16_t *palette) : crt_(crt), palette_(palette) {} - void output_blank(int duration); - void output_sync(int duration); - void output_border(int duration, OutputBpp bpp); - void output_pixels(int duration, OutputBpp bpp); - void output_colour_burst(int duration); + + enum class OutputMode { + Sync, Blank, ColourBurst, Pixels, + }; + + /// Sets the current data format for the shifter. Changes in output BPP flush the shifter. + void set_bpp(OutputBpp bpp); + + void output(int duration, OutputMode mode); + + /// Loads 64 bits into the Shifter. The shifter shifts continuously. If you also declare + /// a pixels region then whatever is being shifted will reach the display, in a form that + /// depends on the current output BPP. void load(uint64_t value); private: + // The target CRT and the palette to use. + Outputs::CRT::CRT &crt_; + uint16_t *palette_ = nullptr; + + // Internal stateful processes. + void generate(int duration, OutputMode mode, bool is_terminal); + + void flush_border(); + void flush_pixels(); + void shift(int duration); + void output_pixels(int duration); + + // Internal state that is a function of output intent. int duration_ = 0; - enum class OutputMode { - Sync, Blank, Border, Pixels, ColourBurst - } output_mode_ = OutputMode::Sync; + OutputMode output_mode_ = OutputMode::Sync; uint16_t border_colour_ = 0; OutputBpp bpp_ = OutputBpp::Four; union { @@ -189,13 +215,9 @@ class Video { uint32_t shifter_halves_[2]; }; - void flush_output(OutputMode next_mode); - + // Internal state for handling output serialisation. uint16_t *pixel_buffer_ = nullptr; - size_t pixel_pointer_ = 0; - - Outputs::CRT::CRT &crt_; - uint16_t *palette_ = nullptr; + int pixel_pointer_ = 0; } video_stream_; /// Contains copies of the various observeable fields, after the relevant propagation delay.