[WIP/POC] Add API to obtain metrics and shape data#856
[WIP/POC] Add API to obtain metrics and shape data#856arch1t3cht wants to merge 2 commits intolibass:masterfrom
Conversation
|
This sounds very good to me! Given the design of this PR, I wonder if anyone would have a use-case for exposing both the images and the metrics from a single render pass (i.e. a way to retrieve the metrics from the just-rendered frame). |
TheOneric
left a comment
There was a problem hiding this comment.
API-wise this seems mostly good to me, some remarks:
Metrics and rendering sharing the same internal process mean they’ll affect each others caches etc and the ass_get_metricsalready mentions the lifetime of metrics thus being affected by render calls. But the reverse is also true and will need to be documented in ass_render_frame; same for changed.
I know you’re not looking for code comments yet, but since it might get relevant in testing: the new API needs to be added to the list of exported symbols, else it only usable from static libs (or broken meson builds).
In prior discussion the distinction between logical and visual metrics came up regularly. Both the issue and this implemented/seem focussed on logical metrics.
Which makes sense since visual extents can already be extracted from rendered bitmaps. I guess providing access to visual and logical metrics in the same call in the future is another reason to keep the internal pipelines for metrics and rendering identical?
This is for debugging the metrics API and should be removed (or possibly replaced with a proper testing program) before merging.
filip-hejsek
left a comment
There was a problem hiding this comment.
I have tried using the POC to implement Aegisub's text metrics API and encountered crashes. Since I have already debugged them and found the cause, I figured I might as well post my analysis here as a review.
|
|
||
| render_and_combine_glyphs(state, device_x, device_y); | ||
|
|
||
| memset(event_images, 0, sizeof(*event_images)); |
There was a problem hiding this comment.
This initialization is skipped if this function returns early.
| if (ass_render_event(&priv->state, event, priv->eimg + cnt)) | ||
| (priv->eimg + cnt)->metrics.runs = NULL; | ||
| if (ass_render_event(&priv->state, event, priv->eimg + cnt, collect_metrics)) |
There was a problem hiding this comment.
This only initializes the metrics partialy, which is insufficient.
| if (!priv->eimg_size) | ||
| return; |
There was a problem hiding this comment.
This only checks if EventImages have been allocated, not initialized.
| // are hence allocated and freed by ass_render_frame_internal. | ||
| // All contained pointers are owned by the ASS_Metrics struct | ||
| // and need to be freed here. | ||
| for (ASS_Metrics *metrics = &priv->eimg[0].metrics; metrics; metrics = metrics->next) { |
There was a problem hiding this comment.
Here we crash because metrics->next is uninitialized.
This is an initial proof-of-concept attempt to implement an API along the lines of #825. It's not yet complete, but before spending more time on this I'd first like to ask if you would be open to adding such a metrics API at all, and if so, if you agree with the overall structure used in this PR. If that is the case, I can polish the PR and discuss some of the finer details (like what fields should be exposed, what data structures to use, and other implementation details).
A couple of comments.
ass_get_metricsfunction that internally does all the same operations asass_render_frame(even including rasterization), but in the end returns shape data rather than bitmaps. The idea being that this is fairly easy to implement from libass's side and provides a very powerful API to the user. (To clarify, I'm imagining the user either creating a dedicatedASS_Trackto pass toass_get_metrics, or to add an extra event they want to measure to an existing track (which they might need if they care about collision detection) and picking out theASS_Metricsstruct pointing to that added event.) A downside may be that the user will need to create an entireASS_Trackwith headers and styles and everything in order to obtain metrics, but since this API is mainly aimed at authoring tools, most programs that would use such an API should already have the machinery for this.ass_get_metricsstill runs the full rasterization code even though the result is not used (though this could also be avoided with more work). This being an authoring API, performance should not be as critical, and I feel like this way it should have better maintainability.Again, the main feedback I'm interested in right now is whether you would consider adding this kind of API at all. If so, I can flesh this out further.