Massive Library Restructuring. #10
Conversation
Takes a string for styles and a string for events and copies them into a script. I think this is a lot better than storing all of the events internally.
It just ends in tears and sadness.
There is no reason not to.
It will be NULL if there is nothing to render. This might smother some errors too, but I hope not.
Theoretically allows people to actually get info when an error occurs.
| } | ||
|
|
||
| ASSI_EXPORT ASSI_State* assi_init( int width, int height, const char *header, uint32_t headerLength, const char* fontConfigConfig, const char *fontDir ) { | ||
| ASSI_State *state = calloc( 1, sizeof(*state) ); |
There was a problem hiding this comment.
Bad decision.
calloc just adds an expensive multiplication which the compiler hopefully optimizes out by replacing with malloc in case of 1 element.
There was a problem hiding this comment.
Since this function call is used exactly once when the library is instantiated, I'm really not too worried about calloc overhead. Calloc differs from malloc in that it zeros the allocated memory.
From my system manpage:
The calloc() function contiguously allocates enough space for count objects that are size bytes of memory each and returns a pointer to the allocated memory. The allocated memory is filled with bytes of value zero.
I consider this to be a lot cleaner (if slightly slower) than manually setting all the pointers to NULL.
There was a problem hiding this comment.
At the end just a few fiels have to be set to zero, not everything, so to the multiplication comes further needless memory setting, even when the initialization fails.
(I wonder about calloc, in my first days of C programming i didn't read this last phrase, maybe added over time... i'm old :( )
There was a problem hiding this comment.
If it were in a loop, I might be worried about the overhead. As it stands currently (allocating 4 pointers, 6 32-bit integers and 128 bytes for a character array, totaling a whopping 184 bytes (ignoring any alignment overhead) on a 64-bit system, unless structs have some overhead I'm not aware of), I sincerely doubt it makes a single iota of difference compared to, say, initializing libass.
And it allows the removal of 4 extraneous statements at the end of the function body, so I'd consider it a net positive.
|
I wouldn't have put styles & events together and pass the header only to initialization, it's too much designed for your Aegisub macro only. The initialization should just create the object with external circumstances, script parts should be editable to use other scripts or properties/sections for the same frame size & fonts cache. Additionally i keep with the point passing lengths isn't required because ASS is a text format, no null-character to expect in input except at the end. |
|
Looking at it, passing the header to initialization was a bad idea. I htink having a separate I finally agree that passing the lengths is unnecessary. Will push a couple of commits today or tomorrow dealing with these changes. |
|
Rather a setter with upcoming getter functions, making an inspector object capable to be used for multiple scripts / headers / styles / events, sharing the same target video & fonts collection. |
|
I'm not sure I understand what the point of allowing multiple scripts would be. Perhaps it's because I'm locked in on a single implementation goal, but I can't think of a situation where having multiple scripts/headers, etc, would be desirable. And if I can't think of a scenario in which they'd be useful or desirable, I won't be able to implement them in a useful fashion. Perhaps you could provide an example of why being able to have multiple scripts would be convenient? |
It's right there. Really.
It turns out that in certain cases with a large amount of blur, checking the bottommost or rightmost 16 pixels isn't enough. Bumping this to 32 fixes those cases while still avoiding exhaustively searching huge rects.
|
I'm going to merge this in 2 days if there are no major grievances. I'd like to make another release from the official repo. Any redesign can, of course, still happen. But I'd rather have something that works for now. |
Okay, so after claiming I didn't have time to work on this, I had a discussion with @line0, and got fixated on a stupid idea that I decided to implement. So I implemented it. And now I'd like to talk about it.
disclaimer: I am not necessarily suggesting merging this pull request wholesale into master as it contains a lot of changes, some of which may be controversial or bad.
The idea
Initially, the plan was to hack in bounding box calculation which, while somewhat more rigorous than simply checking for a dirty pixel, is also far more useful.
Highlights
Events are no longer stored in the library
After that long discussion about the best way to store events in #7 and talking to @line0 about it, I realized there wasn't even really a good reason to actually store events.
Initially implemented in 18b8f68,
assi_setScripttakes two strings as input (one optionallyNULL) and generates a minimal script with it. Note that the script header is still stored on library init, as it is unlikely to change in between runs.Error strings can now be accessed
const char* assi_getErrorString( ASSI_State* )was added in 3d84636, allowing users to actually access the error string. Additionally, a few internal errors actually set the error string properly.A well-commented example script has been added
OptimASS aside, the easiest way to test the API and document it simultaneously was to write an example for how it is intended to be used. Note that the example is Aegisub-specific, but should be usable by anyone.
Font management
As of e696a84, it is possible for the user to specify a directory that fontconfig should search for fonts. This provides a convenient alternative to fontconfig taking time to cache all of the system's fonts.
Uses libass's caching mechanism
c863c8f implements libass's reporting of sequential lines that are identical. This allows the bounding box to be copied (or moved) rather than having to check for it all over again.
Issues
Lots of code duplication
It seems I am incapable of both implementation and design simultaneously. Or perhaps I am just incapable of design. The loop that does bounds checking is present three times in the source, and the code for checking moved events vs checking new events is very similar.
Doesn't take some short cuts
Based on observation, it seems that if the width (as given by
ASS_Image->w) is not padded to the nearest mod 16 integer (which only seems to happen if a clip is present) then the reported width is the exact dirty rect width. The same holds true for height.However it is possible that there is some weird case that taking this shortcut would not work on.