Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Fixed constness of display list storage. #52705

Merged
merged 1 commit into from
May 10, 2024
Merged

Fixed constness of display list storage. #52705

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
28 changes: 14 additions & 14 deletions display_list/display_list.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ DisplayList::DisplayList(DisplayListStorage&& storage,
rtree_(std::move(rtree)) {}

DisplayList::~DisplayList() {
uint8_t* ptr = storage_.get();
const uint8_t* ptr = storage_.get();
DisposeOps(ptr, ptr + byte_count_);
}

Expand Down Expand Up @@ -142,7 +142,7 @@ class VectorCuller final : public Culler {
};

void DisplayList::Dispatch(DlOpReceiver& receiver) const {
uint8_t* ptr = storage_.get();
const uint8_t* ptr = storage_.get();
Dispatch(receiver, ptr, ptr + byte_count_, NopCuller::instance);
}

Expand All @@ -162,16 +162,16 @@ void DisplayList::Dispatch(DlOpReceiver& receiver,
}
const DlRTree* rtree = this->rtree().get();
FML_DCHECK(rtree != nullptr);
uint8_t* ptr = storage_.get();
const uint8_t* ptr = storage_.get();
std::vector<int> rect_indices;
rtree->search(cull_rect, &rect_indices);
VectorCuller culler(rtree, rect_indices);
Dispatch(receiver, ptr, ptr + byte_count_, culler);
}

void DisplayList::Dispatch(DlOpReceiver& receiver,
uint8_t* ptr,
uint8_t* end,
const uint8_t* ptr,
const uint8_t* end,
Culler& culler) const {
DispatchContext context = {
.receiver = receiver,
Expand Down Expand Up @@ -207,7 +207,7 @@ void DisplayList::Dispatch(DlOpReceiver& receiver,
}
}

void DisplayList::DisposeOps(uint8_t* ptr, uint8_t* end) {
void DisplayList::DisposeOps(const uint8_t* ptr, const uint8_t* end) {
while (ptr < end) {
auto op = reinterpret_cast<const DLOp*>(ptr);
ptr += op->size;
Expand All @@ -234,15 +234,15 @@ void DisplayList::DisposeOps(uint8_t* ptr, uint8_t* end) {
}
}

static bool CompareOps(uint8_t* ptrA,
uint8_t* endA,
uint8_t* ptrB,
uint8_t* endB) {
static bool CompareOps(const uint8_t* ptrA,
const uint8_t* endA,
const uint8_t* ptrB,
const uint8_t* endB) {
// These conditions are checked by the caller...
FML_DCHECK((endA - ptrA) == (endB - ptrB));
FML_DCHECK(ptrA != ptrB);
uint8_t* bulk_start_a = ptrA;
uint8_t* bulk_start_b = ptrB;
const uint8_t* bulk_start_a = ptrA;
const uint8_t* bulk_start_b = ptrB;
while (ptrA < endA && ptrB < endB) {
auto opA = reinterpret_cast<const DLOp*>(ptrA);
auto opB = reinterpret_cast<const DLOp*>(ptrB);
Expand Down Expand Up @@ -310,8 +310,8 @@ bool DisplayList::Equals(const DisplayList* other) const {
if (byte_count_ != other->byte_count_ || op_count_ != other->op_count_) {
return false;
}
uint8_t* ptr = storage_.get();
uint8_t* o_ptr = other->storage_.get();
const uint8_t* ptr = storage_.get();
const uint8_t* o_ptr = other->storage_.get();
if (ptr == o_ptr) {
return true;
}
Expand Down
12 changes: 8 additions & 4 deletions display_list/display_list.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -237,7 +237,9 @@ class DisplayListStorage {
DisplayListStorage() = default;
DisplayListStorage(DisplayListStorage&&) = default;

uint8_t* get() const { return ptr_.get(); }
uint8_t* get() { return ptr_.get(); }

const uint8_t* get() const { return ptr_.get(); }

void realloc(size_t count) {
ptr_.reset(static_cast<uint8_t*>(std::realloc(ptr_.release(), count)));
Expand Down Expand Up @@ -309,6 +311,8 @@ class DisplayList : public SkRefCnt {
return modifies_transparent_black_;
}

const DisplayListStorage& GetStorage() const { return storage_; }
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a public method on DisplayList? What is it used for? The storage is supposed to be internal only.

Copy link
Member Author

Choose a reason for hiding this comment

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

Now that storage is immutable there is no harm making it public. This PR was pulled out from #52715 which stores the display list. It allows people to author tools that inspect the display list.

Copy link
Contributor

Choose a reason for hiding this comment

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

The storage has always been immutable in practice (except for the one mutable field in it which will survive these changes because a mutable field is not disabled by a const cast) due to not being exposed. Exposing the buffer as a const pointer would not require such extensive changes and would have had the same external effect. But, I don't want to see this exposed in any case, because...

The harm by making it public is to invite the concept that the data within it is meaningful in any way outside of the DL implementation itself. The implementation can and does change frequently, so the stream of data is not meaningful in any lasting (or even temporary) manner. By exposing it, it's as if we're promising reusability or meaningfulness for the data that is exposed and both of those are (while not stated in the doc comments) deliberately not supported.

This is not a substitute for a serialization/readback mechanism and isn't really useful as a manual inspection tool especially in light of the fact that there exists a supported pretty printing tool for DisplayLists in

engine/testing/display_list_testing.h

Line 33 in 900ca9a

extern std::ostream& operator<<(std::ostream& os,


private:
DisplayList(DisplayListStorage&& ptr,
size_t byte_count,
Expand All @@ -324,7 +328,7 @@ class DisplayList : public SkRefCnt {

static uint32_t next_unique_id();

static void DisposeOps(uint8_t* ptr, uint8_t* end);
static void DisposeOps(const uint8_t* ptr, const uint8_t* end);

const DisplayListStorage storage_;
const size_t byte_count_;
Expand All @@ -345,8 +349,8 @@ class DisplayList : public SkRefCnt {
const sk_sp<const DlRTree> rtree_;

void Dispatch(DlOpReceiver& ctx,
uint8_t* ptr,
uint8_t* end,
const uint8_t* ptr,
const uint8_t* end,
Culler& culler) const;

friend class DisplayListBuilder;
Expand Down