[MPlayer-dev-eng] Re: [PATCH] SSA/ASS subtitles support

[Uoti Urpala]

I think the patch could be applied soon and further fixes done in svn. 
One thing I'd personally like to change before applying it is the
indentation style of new files. MPlayer has traditionally added new
files using whatever indentation style the author preferred, but as
maintainers of files change over time and other people usually have to
at least read the files I don't particularly like the end result. My
idea of the most agreeable common style is basically that produced by
"indent -kr", with the only significant difference from the current
files being an indentation level of 4 spaces instead of the current 8. 
If you're going to keep working on the files and strongly prefer 8
spaces I don't have major objections against that (I don't find the
files hard to read), but would prefer a 4-space indent.


Some more comments about the latest version:

+static struct vf_priv_s {
+	int outh, outw;
+
+	unsigned int outfmt;
+
+	// 1 = auto-added filter: insert only if chain does not support EOSD
already
+	// 0 = insert always
+	int auto_insert;
+
+	ass_instance_t* ass_priv;
+
+	unsigned char* planes[3];
+	unsigned char* dirty_rows;
+} vf_priv_dflt = {
+	0, 0,
+
+	0,
+
+	0,
+
+	NULL,
+
+	{0, 0, 0},
+	0
+};

You could just rely on the default initialization to 0 for static
variables, and I think it would be clearer in this case. Various
pointers in this data structure are filled with malloced values and
apparently leaked when playing multiple files.


+#define skip(x) if (*p == (x)) ++p; else { return p; }

I think it would be nice to print a diagnostic (at least at verbose
level, maybe higher) if some parts are skipped because of parsing
errors, especially as not quite all features are yet supported. I had
some subtitles using the 6-argument version of \move which became
completely invisible due to clipping when the move was ignored, now
there was no indication that anything might be missing.

The specs I have say "Any style modifier followed by no recognizable
parameter resets to the default". This doesn't seem to be handled
correctly. I have subtitles that use {\c} to reset to the default color,
now the displayed line is cut at that location.

+void ass_face_cache_done();
+void ass_glyph_cache_done();
+ass_instance_t* ass_init();
+ass_image_t* ass_end_frame(); // returns linked list of images to render
+ass_track_t* ass_new_track();

Old-style declarations, should have "void" as parameter list.