Speedup semantic rasterizer#140
Speedup semantic rasterizer#140pestipeti wants to merge 5 commits intowoven-by-toyota:masterfrom pestipeti:speedup_semantic_rasterizer
Conversation
lucabergamini
left a comment
lucabergamini
left a comment
There was a problem hiding this comment.
Amazing work! minors, you can either pick them up yourself or I can also do them, just let me know :)
| ) | ||
|
|
||
| return {"xyz": xyz} | ||
| xyz[:, -1] = 1.0 |
There was a problem hiding this comment.
why is this required? We ignore the z coordinates in the following so this shouldn't make any difference right?
There was a problem hiding this comment.
In the current implementation, first you cut the z-coordinate lane_coords["xyz_right"][:, :2] and later in the transform method you stack it back np.vstack((points[:num_dims, :], np.ones(points.shape[1]))). With this we can save the one cut, the np.ones creation, the stack.
There was a problem hiding this comment.
Another way of doing this would be to only use the first 2x2 of the matrix (only XY) in the semantic rasterizer. My issue with setting 1 here is that we're removing information in a very hidden part of the code, which may render debugging problematic in the future (also considering there is a cache system in between)
There was a problem hiding this comment.
I see. You can move it to the semantic rasterizer method; before the dot product calls.
There was a problem hiding this comment.
I like this idea also :) Today was quite busy, but I should be able to work on this tomorrow (hopefully)
|
|
||
| return {"xyz": xyz} | ||
| xyz[:, -1] = 1.0 | ||
| return {"xyz": xyz.T} |
There was a problem hiding this comment.
I can see the point of caching this to avoid stack at runtime, but we may still want to include either the two lanes or at least the length of the first, so that we can always unpack the two apart (I'm thinking about centre line support right now)
There was a problem hiding this comment.
The cheapest/easiest solution if you add the length of the first line. I think map_api should handle the centerline calculation as well (cachable).
There was a problem hiding this comment.
yeah, I agree on that and I'm fine with having the length included
There was a problem hiding this comment.
One note: I changed the returned xyz format to xyz.T.
| xy_left = cv2_subpixel(transform_points(lane_coords["xyz_left"][:, :2], world_to_image_space)) | ||
| xy_right = cv2_subpixel(transform_points(lane_coords["xyz_right"][:, :2], world_to_image_space)) | ||
| lanes_area = np.vstack((xy_left, np.flip(xy_right, 0))) # start->end left then end->start right | ||
| lanes_xy = cv2_subpixel(world_to_image_space.dot(lane_coords["xyz"]).T[:, :2]) |
There was a problem hiding this comment.
Why not using transform or transform_transpose functions here?
There was a problem hiding this comment.
It seemed a bit complicated. Transposes, vstack, np.ones, cut z-axis, etc. I did not want to break other parts of the code by modifying those methods.
(Feel free to correct anything you want.)
There was a problem hiding this comment.
Yeah you're perfectly right about that. Let me see if we can also handle this case there (same len for matrix and points)
There was a problem hiding this comment.
I did these modifications only to save as many CPU cycles as I possibly can. I did not consider long term usability, I only care about speeding up for the competition, so it is fine if you drop all of or part of this PR.
|
render_semantic_map is the bottleneck when using a low count of history_frames. |
did you compare this against #196? |
thanks for pointing to that PR. It does even better once the caching is done: 52 ms ± 4.77 ms per loop (mean ± std. dev. of 7 runs, 10 loops each) no changes 32.9 ms ± 17.2 ms per loop (mean ± std. dev. of 7 runs, 10 loops each) PR #196 after caching: timings are for the full dataset loading (448x224 raster, 0 history frames) so the speedups are normalized to include all other operations such as box_rasterizer as well) |
4 participants
I achieved ~ 7-8% speedup with these modifications.