Fix rendering hashes, empty bounds, and script preservation

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 = { }

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:" ) ) {

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;
 				}

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

tests/meson.build

@@ -4,14 +4,20 @@ 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()
         wrapper = custom_target('Inspector.lua',
             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')],

tests/render.c

@@ -22,14 +22,28 @@ 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);
     CHECK(rects[0].x == rects[1].x && rects[0].y == rects[1].y);
     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);

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.')