Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 20 additions & 2 deletions impeller/tessellator/tessellator.cc
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ static void HeapFree(void* userData, void* ptr) {
}

// Note: these units are "number of entities" for bucket size and not in KB.
static TESSalloc alloc = {
static const TESSalloc kAlloc = {
HeapAlloc, HeapRealloc, HeapFree, 0, /* =userData */
16, /* =meshEdgeBucketSize */
16, /* =meshVertexBucketSize */
Expand All @@ -32,7 +32,19 @@ static TESSalloc alloc = {
};

Tessellator::Tessellator()
: c_tessellator_(::tessNewTess(&alloc), &DestroyTessellator) {}
: c_tess_alloc_(nullptr, &DestroyTessAlloc),
c_tessellator_(nullptr, &DestroyTessellator) {
{

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This nested scope can be removed

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I used those so someone doesn't accidentally use tessellator after the move. That's not a compilation error, but is a clang tidy error.

CTessAlloc tess_alloc(new TESSalloc(), &DestroyTessAlloc);
c_tess_alloc_ = std::move(tess_alloc);
}
memcpy(c_tess_alloc_.get(), &kAlloc, sizeof(TESSalloc));
{
CTessellator tessellator(::tessNewTess(c_tess_alloc_.get()),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tessNewTess copies the TESSalloc contents into the newly created tessellator and does not mutate the alloc argument:
https://flutter.googlesource.com/third_party/libtess2/+/refs/heads/master/Source/tess.h#84
https://flutter.googlesource.com/third_party/libtess2/+/refs/heads/master/Source/tess.c#610

If you want to clarify the lifetime of this structure, then it should be sufficient to declare the TESSalloc as a local variable here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. The code as it was written is conforming to the api and docstrings since it takes a mutable pointer and has no mention of what it does to it. libTess2 will never be updated so it should be safe to pass a pointer to something on the stack.

&DestroyTessellator);
c_tessellator_ = std::move(tessellator);
}
}

Tessellator::~Tessellator() = default;

Expand Down Expand Up @@ -128,4 +140,10 @@ void DestroyTessellator(TESStesselator* tessellator) {
}
}

void DestroyTessAlloc(TESSalloc* alloc) {
if (alloc != nullptr) {
delete alloc;
}
}

} // namespace impeller
6 changes: 6 additions & 0 deletions impeller/tessellator/tessellator.h
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
#include "impeller/geometry/point.h"

struct TESStesselator;
struct TESSalloc;

namespace impeller {

Expand All @@ -21,6 +22,10 @@ void DestroyTessellator(TESStesselator* tessellator);
using CTessellator =
std::unique_ptr<TESStesselator, decltype(&DestroyTessellator)>;

void DestroyTessAlloc(TESSalloc* alloc);

using CTessAlloc = std::unique_ptr<TESSalloc, decltype(&DestroyTessAlloc)>;

enum class WindingOrder {
kClockwise,
kCounterClockwise,
Expand Down Expand Up @@ -64,6 +69,7 @@ class Tessellator {
const BuilderCallback& callback) const;

private:
CTessAlloc c_tess_alloc_;
CTessellator c_tessellator_;

FML_DISALLOW_COPY_AND_ASSIGN(Tessellator);
Expand Down