Document //flutter/shell/common/rasterizer #9809
Conversation
shell/common/rasterizer.h
Outdated
| /// to it by the engine (which lives on the UI task runner). | ||
| /// | ||
| /// The primary components owned by the rasterizer are the compositor context | ||
| /// and the on-screen render surface. The compositor context all the GPU state |
There was a problem hiding this comment.
"The compositor context HAS/SAVES all the GPU state"?
shell/common/rasterizer.h
Outdated
| /// | ||
| /// The rasterizer owns the instance of the currently active on-screen render | ||
| /// surface. On this surface, it renders the contents of layer trees submitted | ||
| /// to it by the engine (which lives on the UI task runner). |
There was a problem hiding this comment.
Maybe explicitly specify flutter::Engine so readers won't be confused with the engine as a whole (e.g., flutter/engine repo).
| /// held till the balancing call to `Rasterizer::Teardown` is | ||
| /// made. Calling a setup before tearing down the previous surface | ||
| /// (if this is not the first time the surface has been setup) is | ||
| /// user error. |
There was a problem hiding this comment.
I think this usage is correct.
| /// @brief Releases the previously setup on-screen render surface and | ||
| /// collects associated resources. No more rendering may occur | ||
| /// till the next call to `Rasterizer::Setup` with a new render | ||
| /// surface. Calling a teardown without a setup is user error. |
There was a problem hiding this comment.
I think this usage is correct.
| void NotifyLowMemoryWarning() const; | ||
|
|
||
| //---------------------------------------------------------------------------- |
There was a problem hiding this comment.
Maybe add "The weak pointer can be copied to other threads, but the rasterizer it references to may only be accessed..."
There was a problem hiding this comment.
Isn't that a property of all weak pointers? That is documented in FML. This weak pointer is not special in any way.
shell/common/rasterizer.h
Outdated
| fml::WeakPtr<Rasterizer> GetWeakPtr() const; | ||
|
|
||
| //---------------------------------------------------------------------------- | ||
| /// @brief Gets a weak pointer to the rasterizer. The rasterizer is also |
There was a problem hiding this comment.
I guess this is no longer needed if #9813 lands first?
There was a problem hiding this comment.
I'll remove it then.
| /// layer tree describing the same contents. One such case is when | ||
| /// external textures (video or camera streams for example) are | ||
| /// updated in an otherwise static layer tree. To support this use | ||
| /// case, the rasterizer holds onto the last rendered layer tree. |
There was a problem hiding this comment.
Maybe leave a link to flutter/flutter#33939 to clarify that we intend to reduce such redundant rendering.
There was a problem hiding this comment.
Done. I added an @bug line.
shell/common/rasterizer.h
Outdated
| /// again with new limits after surface re-acquisition. | ||
| /// | ||
| /// @attention This cache does not describe the entirety of GPU resources | ||
| /// that may be cached. The raster cache also holds very large GPU |
There was a problem hiding this comment.
Maybe add a reference to |RasterCache| (so they know that raster cache is not just a concept, but an actual class)?
| /// a frame on the on-screen surface. | ||
| /// | ||
| /// Why does the draw call take a layer tree pipeline and not the | ||
| /// layer tree directly? |
There was a problem hiding this comment.
Maybe add a note like "|Rasterizer::DoDraw| takes a layer tree and rasterizes it; The |Rasterizer::Draw| takes the pipeline as it needs to do additional pipeline maintenance work".
chinmaygarde
force-pushed
the
document_rasterizer
branch
from
ef206c2 to
90ba4de
Compare
|
Rebased on ToT. |
No description provided.