Skip to content

Commit

Permalink
Fix bugprone-unchecked-optional-access errors in image_generator_apng (
Browse files Browse the repository at this point in the history
…flutter#42450)

Related to #127701

When landing a seemingly unrelated CL, clang-tidy started failing with:

```
❌ Failures for clang-tidy on /b/s/w/ir/cache/builder/src/flutter/lib/ui/painting/image_generator_apng.cc:
/b/s/w/ir/cache/builder/src/flutter/lib/ui/painting/image_generator_apng.cc:57:10: error: unchecked access to optional value [bugprone-unchecked-optional-access,-warnings-as-errors]
  return images_[image_index].frame_info.value();
         ^
/b/s/w/ir/cache/builder/src/flutter/lib/ui/painting/image_generator_apng.cc:154:13: error: unchecked access to optional value [bugprone-unchecked-optional-access,-warnings-as-errors]
    switch (frame.frame_info->blend_mode) {
            ^
/b/s/w/ir/cache/builder/src/flutter/lib/ui/painting/image_generator_apng.cc:526:7: error: unchecked access to optional value [bugprone-unchecked-optional-access,-warnings-as-errors]
  if (images_.back().frame_info->disposal_method ==
      ^
/b/s/w/ir/cache/builder/src/flutter/lib/ui/painting/image_generator_apng.cc:535:7: error: unchecked access to optional value [bugprone-unchecked-optional-access,-warnings-as-errors]
      images_.back().frame_info->disposal_method ==
      ^
/b/s/w/ir/cache/builder/src/flutter/lib/ui/painting/image_generator_apng.cc:538:5: error: unchecked access to optional value [bugprone-unchecked-optional-access,-warnings-as-errors]
    image->frame_info->required_frame = images_.size() - 1;
    ^
Suppressed 1772 warnings (1772 in non-user code).
```

This addresses those checks by making sure frame_info has a value.

## Pre-launch Checklist

- [x] I read the [Contributor Guide] and followed the process outlined
there for submitting PRs.
- [x] I read the [Tree Hygiene] wiki page, which explains my
responsibilities.
- [x] I read and followed the [Flutter Style Guide] and the [C++,
Objective-C, Java style guides].
- [x] I listed at least one issue that this PR fixes in the description
above.
- [x] I added new tests to check the change I am making or feature I am
adding, or Hixie said the PR is test-exempt. See [testing the engine]
for instructions on writing and running engine tests.
- [ ] I updated/added relevant documentation (doc comments with `///`).
- [x] I signed the [CLA].
- [ ] All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel
on [Discord].

<!-- Links -->
[Contributor Guide]:
https://github.com/flutter/flutter/wiki/Tree-hygiene#overview
[Tree Hygiene]: https://github.com/flutter/flutter/wiki/Tree-hygiene
[Flutter Style Guide]:
https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo
[C++, Objective-C, Java style guides]:
https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
[testing the engine]:
https://github.com/flutter/flutter/wiki/Testing-the-engine
[CLA]: https://cla.developers.google.com/
[flutter/tests]: https://github.com/flutter/tests
[breaking change policy]:
https://github.com/flutter/flutter/wiki/Tree-hygiene#handling-breaking-changes
[Discord]: https://github.com/flutter/flutter/wiki/Chat
  • Loading branch information
kjlubick authored May 31, 2023
1 parent ee53c04 commit dda0e47
Showing 1 changed file with 19 additions and 5 deletions.
24 changes: 19 additions & 5 deletions lib/ui/painting/image_generator_apng.cc
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
#include "third_party/skia/include/codec/SkCodecAnimation.h"
#include "third_party/skia/include/core/SkAlphaType.h"
#include "third_party/skia/include/core/SkColorType.h"
#include "third_party/skia/include/core/SkImageInfo.h"
#include "third_party/skia/include/core/SkStream.h"
#include "third_party/zlib/zlib.h" // For crc32

Expand Down Expand Up @@ -54,7 +55,10 @@ const ImageGenerator::FrameInfo APNGImageGenerator::GetFrameInfo(
return {};
}

return images_[image_index].frame_info.value();
if (images_[image_index].frame_info.has_value()) {
return images_[image_index].frame_info.value();
}
return {};
}

SkISize APNGImageGenerator::GetScaledDimensions(float desired_scale) {
Expand Down Expand Up @@ -85,7 +89,7 @@ bool APNGImageGenerator::GetPixels(const SkImageInfo& info,
///

APNGImage& frame = images_[image_index];
auto frame_info = frame.codec->getInfo();
SkImageInfo frame_info = frame.codec->getInfo();
auto frame_row_bytes = frame_info.bytesPerPixel() * frame_info.width();

if (frame.pixels.empty()) {
Expand All @@ -99,6 +103,12 @@ bool APNGImageGenerator::GetPixels(const SkImageInfo& info,
return RenderDefaultImage(info, pixels, row_bytes);
}
}
if (!frame.frame_info.has_value()) {
FML_DLOG(ERROR) << "Failed to decode image at index " << image_index
<< " (frame index: " << frame_index
<< ") of APNG due to the frame missing data (frame_info).";
return false;
}

//----------------------------------------------------------------------------
/// 3. Composite the frame onto the canvas.
Expand Down Expand Up @@ -519,11 +529,15 @@ bool APNGImageGenerator::DemuxNextImageInternal() {
const void* data_p = const_cast<void*>(data_.get()->data());
std::tie(image, next_chunk_p_) =
DemuxNextImage(data_p, data_->size(), header_, next_chunk_p_);
if (!image.has_value()) {
if (!image.has_value() || !image->frame_info.has_value()) {
return false;
}

if (images_.back().frame_info->disposal_method ==
auto last_frame_info = images_.back().frame_info;
if (!last_frame_info.has_value()) {
return false;
}
if (last_frame_info->disposal_method ==
SkCodecAnimation::DisposalMethod::kRestorePrevious) {
FML_DLOG(INFO)
<< "DisposalMethod::kRestorePrevious is not supported by the "
Expand All @@ -532,7 +546,7 @@ bool APNGImageGenerator::DemuxNextImageInternal() {
}

if (images_.size() > first_frame_index_ &&
images_.back().frame_info->disposal_method ==
last_frame_info->disposal_method ==
SkCodecAnimation::DisposalMethod::kKeep) {
// Mark the required frame as the previous frame in all cases.
image->frame_info->required_frame = images_.size() - 1;
Expand Down

0 comments on commit dda0e47

Please sign in to comment.