From 54e7888caa3766ed4591cd79cd52a8bb9ab919de Mon Sep 17 00:00:00 2001 From: Ryan Lucia Date: Fri, 11 Sep 2026 18:51:27 -0700 Subject: [PATCH] Fix rendering hashes, empty bounds, and script preservation --- examples/Aegisub/Inspector.moon | 7 ++-- examples/C++/example.cpp | 14 ++++++-- src/SubInspector.c | 13 +++++--- tests/example.py | 24 ++++++++++++++ tests/meson.build | 8 ++++- tests/render.c | 57 ++++++++++++++++++++++++++++++++- tests/wrapper.lua | 41 ++++++++++++++++++++++-- 7 files changed, 151 insertions(+), 13 deletions(-) create mode 100644 tests/example.py diff --git a/examples/Aegisub/Inspector.moon b/examples/Aegisub/Inspector.moon index acdd116..fde1988 100644 --- a/examples/Aegisub/Inspector.moon +++ b/examples/Aegisub/Inspector.moon @@ -103,14 +103,17 @@ collectHeader = ( subtitles ) => "[Script Info]" } - -- These are the only header fields that actually affect the way ASS - -- is rendered. I don't actually know if ScriptType matters. + -- Preserve script settings that affect libass rendering. infoFields = { PlayResX: true PlayResY: true WrapStyle: true ScriptType: true ScaledBorderAndShadow: true + Kerning: true + Language: true + LayoutResX: true + LayoutResY: true } styles = { } diff --git a/examples/C++/example.cpp b/examples/C++/example.cpp index c49e5ee..b1b554a 100644 --- a/examples/C++/example.cpp +++ b/examples/C++/example.cpp @@ -16,6 +16,14 @@ struct DialogLine { std::string line; }; +static std::istream& readLine( std::istream &stream, std::string &line ) { + std::getline( stream, line ); + if ( !line.empty( ) && line.back( ) == '\r' ) { + line.pop_back( ); + } + return stream; +} + class ScriptWrapper { std::string headerBuffer; std::fstream &fileStream; @@ -42,7 +50,7 @@ ScriptWrapper::~ScriptWrapper( void ) { void ScriptWrapper::readFile( int &width, int &height ) { std::string line; - std::getline( fileStream, line ); + readLine( fileStream, line ); if ( "[Script Info]" != line ) { // BOM? if ( "[Script Info]" != line.substr( 3 ) ) { @@ -54,7 +62,7 @@ void ScriptWrapper::readFile( int &width, int &height ) { // This is pretty terrible. std::cout << "Reading headers." << std::endl; while ( true ) { - std::getline( fileStream, line ); + readLine( fileStream, line ); if ( '[' == line[0] ) { break; } @@ -95,7 +103,7 @@ void ScriptWrapper::readFile( int &width, int &height ) { if ( !fileStream.good( ) ) goto fail; - std::getline( fileStream, line ); + readLine( fileStream, line ); switch ( line[0] ) { case 'S': if ( 0 == line.compare( 0, 6, "Style:" ) ) { diff --git a/src/SubInspector.c b/src/SubInspector.c index 596cd73..24e5b99 100644 --- a/src/SubInspector.c +++ b/src/SubInspector.c @@ -198,6 +198,7 @@ int si_calculateBounds( SI_State *state, SI_Rect *rects, const int32_t *times, c ASS_Image *assImage = ass_render_frame( state->assRenderer, assTrack, times[i], &lineChanged ); if ( NULL == assImage ) { + rects[i] = (SI_Rect){0}; continue; } @@ -231,6 +232,8 @@ int si_calculateBounds( SI_State *state, SI_Rect *rects, const int32_t *times, c continue; } if ( 0xFF != (assImage->color & 0xFF) ) { + const int imageBounds[] = {assImage->dst_x, assImage->dst_y, assImage->w, assImage->h}; + pixelHash = crc32( pixelHash, (const void *)imageBounds, sizeof(imageBounds) ); solid = solid | checkBounds( assImage, &boundsRect, &pixelHash ); } pixelHash = crc32( pixelHash, (void*)&assImage->color, sizeof(assImage->color) ); @@ -246,7 +249,9 @@ int si_calculateBounds( SI_State *state, SI_Rect *rects, const int32_t *times, c rects[i].w = boundsRect.x2 - boundsRect.x1; rects[i].h = boundsRect.y2 - boundsRect.y1; rects[i].solid = solid; - rects[i].hash = crc32( pixelHash, (void*)&rects[i], sizeof(rects[i]) ); + // All rendered image data is already hashed; never hash output storage, + // which contains the caller's previous hash and possibly unset padding. + rects[i].hash = pixelHash; state->lastRect = rects[i]; } @@ -280,6 +285,9 @@ static uint8_t checkBounds( ASS_Image *assImage, SI_InternalRect *boundsRect, ui const uint32_t chunksPerRow = assImage->w/chunkSize; while ( byte < bitmapEnd ) { + // Include zero pixels so that gaps and row boundaries retain their + // positions. Exclude stride padding, including the unpadded last row. + *pixelHash = crc32( *pixelHash, byte, assImage->w ); chunk = (uintptr_t *)byte; addHeight = 0; @@ -296,7 +304,6 @@ static uint8_t checkBounds( ASS_Image *assImage, SI_InternalRect *boundsRect, ui // printf( "Chunk: %p; Value: %016lX, End: %p\n", chunk, *chunk, chunk + 1 ); for( byte = (uint8_t *)chunk; byte < (uint8_t *)(chunk + 1); byte++ ) { if ( *byte ) { - *pixelHash = crc32( *pixelHash, (void *)byte, 1 ); if ( *byte == 255 ) { solid = 1; } @@ -320,8 +327,6 @@ static uint8_t checkBounds( ASS_Image *assImage, SI_InternalRect *boundsRect, ui byte = (uint8_t *)chunk; while ( byte < endByte ) { if ( *byte ) { - *pixelHash = crc32( *pixelHash, (void *)byte, 1 ); - if ( *byte == 255 ) { solid = 1; } diff --git a/tests/example.py b/tests/example.py new file mode 100644 index 0000000..a83d4b9 --- /dev/null +++ b/tests/example.py @@ -0,0 +1,24 @@ +"""The example accepts ASS files with either newline style, with or without a BOM.""" + +import subprocess +import sys +import tempfile +from pathlib import Path + +script = """[Script Info] +ScriptType: v4.00+ +PlayResX: 640 +PlayResY: 480 +[V4+ Styles] +Style: Default,Arial,20,&H00FFFFFF,&H000000FF,&H00000000,&H00000000,0,0,0,0,100,100,0,0,1,0,0,7,0,0,0,1 +[Events] +Dialogue: 0,0:00:00.00,0:00:02.00,Default,,0,0,0,,{\\an7\\pos(100,100)\\p1}m 0 0 l 20 0 20 20 0 20 +""" + +with tempfile.TemporaryDirectory(prefix="SubInspector example ") as directory: + path = Path(directory) / "sample.ass" + for newline in ("\n", "\r\n"): + for bom in (b"", b"\xef\xbb\xbf"): + path.write_bytes(bom + script.replace("\n", newline).encode("utf-8")) + result = subprocess.run([sys.argv[1], str(path)], capture_output=True, text=True, check=True) + assert "Deleted 0 lines out of 1" in result.stdout, result.stdout diff --git a/tests/meson.build b/tests/meson.build index d9e6db3..533a08e 100644 --- a/tests/meson.build +++ b/tests/meson.build @@ -4,6 +4,13 @@ render_test = executable('render-test', 'render.c', test('render', render_test) if not meson.is_cross_build() + python = import('python').find_installation() + if add_languages('cpp', required: false, native: false) + example = executable('si-example', '../examples/C++/example.cpp', + override_options: ['cpp_std=c++11'], link_with: subinspector) + test('example-line-endings', python, + args: [files('example.py'), example.full_path()], depends: example) + endif luajit = find_program('luajit', required: false) moonc = find_program('moonc', required: false) if luajit.found() and moonc.found() @@ -11,7 +18,6 @@ if not meson.is_cross_build() input: '../examples/Aegisub/Inspector.moon', output: 'Inspector.lua', command: [moonc, '-o', '@OUTPUT@', '@INPUT@']) - python = import('python').find_installation() test('aegisub-wrapper', python, args: [files('wrapper.py'), luajit.full_path(), wrapper, subinspector.full_path(), files('wrapper.lua')], diff --git a/tests/render.c b/tests/render.c index e563745..cebf05a 100644 --- a/tests/render.c +++ b/tests/render.c @@ -22,7 +22,8 @@ static const char header[] = static SI_Rect render(SI_State *state, const char *script) { const int32_t times[] = {0, 500, 2500}; - SI_Rect rects[3] = {{0}}; + SI_Rect rects[3]; + memset(rects, 0xa5, sizeof(rects)); CHECK(si_setScript(state, script, 0) == 0); CHECK(si_calculateBounds(state, rects, times, 3) == 0); CHECK(rects[0].w > 0 && rects[0].h > 0); @@ -30,6 +31,19 @@ static SI_Rect render(SI_State *state, const char *script) { CHECK(rects[0].w == rects[1].w && rects[0].h == rects[1].h); CHECK(rects[0].hash == rects[1].hash); CHECK(rects[2].w == 0 && rects[2].h == 0); + CHECK(rects[2].x == 0 && rects[2].y == 0); + CHECK(rects[2].hash == 0 && rects[2].solid == 0); + + /* Reuse the populated output array, then exercise the unchanged-frame + cache with an output buffer containing different bytes. */ + CHECK(si_calculateBounds(state, rects, times, 3) == 0); + SI_Rect repeated; + memset(&repeated, 0x5a, sizeof(repeated)); + CHECK(si_calculateBounds(state, &repeated, times, 1) == 0); + CHECK(rects[0].hash == repeated.hash); + memset(&repeated, 0x3c, sizeof(repeated)); + CHECK(si_calculateBounds(state, &repeated, times, 1) == 0); + CHECK(rects[0].hash == repeated.hash); return rects[0]; } @@ -46,6 +60,15 @@ int main(void) { CHECK(rect.solid == 1); CHECK(render(state, shape).hash == rect.hash); + /* A script without events can leave libass's previous image cache in + place. Clear its result without losing the cached visible rectangle. */ + CHECK(si_setScript(state, "\n", 0) == 0); + const int32_t time = 0; + SI_Rect empty = rect; + CHECK(si_calculateBounds(state, &empty, &time, 1) == 0); + CHECK(empty.w == 0 && empty.h == 0 && empty.hash == 0); + CHECK(render(state, shape).hash == rect.hash); + /* Change only the first of two rendered images. This catches the old bug where only the final image contributed to the pixel hash. */ const char *two_shapes = @@ -64,6 +87,38 @@ int main(void) { CHECK(original.w == changed.w && original.h == changed.h); CHECK(original.hash != changed.hash); + /* Moving an interior image must change the hash even though neither + its bitmap nor the combined bounds change. */ + const char three_shapes[] = + "Dialogue: 0,0:00:00.00,0:00:02.00,Default,,0,0,0,," + "{\\an7\\pos(100,100)\\p1}m 0 0 l 20 0 20 20 0 20\n" + "Dialogue: 1,0:00:00.00,0:00:02.00,Default,,0,0,0,," + "{\\an7\\pos(140,100)\\p1}m 0 0 l 20 0 20 20 0 20\n" + "Dialogue: 2,0:00:00.00,0:00:02.00,Default,,0,0,0,," + "{\\an7\\pos(200,100)\\p1}m 0 0 l 20 0 20 20 0 20\n"; + char moved_middle[sizeof(three_shapes)]; + strcpy(moved_middle, three_shapes); + char *position = strstr(moved_middle, "140,100"); + CHECK(position != NULL); + memcpy(position, "160,100", 7); + original = render(state, three_shapes); + changed = render(state, moved_middle); + CHECK(original.x == changed.x && original.y == changed.y); + CHECK(original.w == changed.w && original.h == changed.h); + CHECK(original.hash != changed.hash); + + /* Equal-area drawings with the same bounds but different gaps must + differ too: hashing only nonzero pixels loses their arrangement. */ + original = render(state, + "Dialogue: 0,0:00:00.00,0:00:02.00,Default,,0,0,0,," + "{\\an7\\pos(100,100)\\p1}m 0 0 l 8 0 8 4 0 4 m 12 8 l 20 8 20 12 12 12\n"); + changed = render(state, + "Dialogue: 0,0:00:00.00,0:00:02.00,Default,,0,0,0,," + "{\\an7\\pos(100,100)\\p1}m 12 0 l 20 0 20 4 12 4 m 0 8 l 8 8 8 12 0 12\n"); + CHECK(original.x == changed.x && original.y == changed.y); + CHECK(original.w == changed.w && original.h == changed.h); + CHECK(original.hash != changed.hash); + /* Exercise the platform font provider and shaping dependencies. */ render(state, "Dialogue: 0,0:00:00.00,0:00:02.00,Default,,0,0,0,,Hello SubInspector\n"); si_cleanup(state); diff --git a/tests/wrapper.lua b/tests/wrapper.lua index 2664a6d..ec7b1e8 100644 --- a/tests/wrapper.lua +++ b/tests/wrapper.lua @@ -78,13 +78,13 @@ assert(message:find('wrong architecture', 1, true)) -- with and without DependencyControl registration. local declared = false for _, depctrl in ipairs({false, true}) do - local loaded_path, config + local loaded_path, config, library local test_ffi = setmetatable({ cdef = function(definitions) if not declared then ffi.cdef(definitions); declared = true end end, load = function(path) - local library = ffi.load(path) + library = ffi.load(path) loaded_path = path return setmetatable({ si_init = function(width, height, font_config, fonts) @@ -118,6 +118,43 @@ for _, depctrl in ipairs({false, true}) do assert(rects[1].hash == rects[2].hash) local configured = Inspector(subtitles, 'custom-fonts.conf') assert(config == 'custom-fonts.conf' and configured.fcConfig == config) + + -- Compare rendering settings with a native render of the complete + -- script, so the wrapper cannot silently change subtitle appearance. + local settings = { + {'Kerning', 'yes'}, {'Language', 'tr'}, + {'LayoutResX', '1280'}, {'LayoutResY', '720'}, + } + for _, setting in ipairs(settings) do + table.insert(subtitles, #subtitles, { + class = 'info', key = setting[1], value = setting[2], + raw = setting[1] .. ': ' .. setting[2], + }) + end + assert(inspector:updateHeader(subtitles)) + local full_header = {'[Script Info]'} + for _, line in ipairs(subtitles) do + if line.class == 'style' then + table.insert(full_header, '[V4+ Styles]') + else + assert(inspector.header:find(line.raw, 1, true)) + end + table.insert(full_header, line.raw) + end + table.insert(full_header, '[Events]\n') + local native = ffi.gc(library.si_init(640, 480, nil, nil), library.si_cleanup) + assert(native ~= nil) + local header = table.concat(full_header, '\n') + assert(library.si_setHeader(native, header, #header) == 0) + local text = '{\\an7\\pos(100,100)\\fs80\\bord2\\blur2}AVAVAV' + local raw = 'Dialogue: 0,0:00:00.00,0:00:02.00,Default,,0,0,0,,' .. text + assert(library.si_setScript(native, raw, #raw) == 0) + local expected = ffi.new('SI_Rect[1]') + assert(library.si_calculateBounds(native, expected, ffi.new('int32_t[1]', 0), 1) == 0) + local actual = assert(inspector:getBounds({{style = 'Default', text = text, raw = raw}}, {0}))[1] + for _, field in ipairs({'x', 'y', 'w', 'h', 'hash'}) do + assert(actual[field] == tonumber(expected[0][field]), field .. ' differs from native rendering') + end end print('Library discovery, errors, DependencyControl registration, and wrapper rendering passed.')