Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
494503002:
Expose IsSolidColor and write units tests (Closed)
DescriptionExpose IsSolidColor and write units tests
AnalysisRaster was needed because we must run the analysis on a separate
thread. My second attempt placed the analysis in the picturepileimpl,
but, as vmpstr predicted, this caused issues when software rasterizing
since it uses yet another thread. The way around all of this was to
write another raster function without the:
DCHECK(raster_thread_checker_.CalledOnValidThread());
We're calling the Analysis before any synchronization occures with impl
so it should be safe to call without the check.
BUG=396908
Committed: https://crrev.com/c60f5fbb953fb80e7f36cb7885e6f6c8b8494439
Cr-Commit-Position: refs/heads/master@{#292044}
Patch Set 1 #Patch Set 2 : initialized some variables #
Total comments: 11
Patch Set 3 : Fixed Vmpstr's issues #
Total comments: 14
Patch Set 4 : Fix for review commetns #Patch Set 5 : See what people think of the ResetThreadChecker approach #Patch Set 6 : It turns out that we could end up with nullptr entries in the map somehow. I hit the dcheck, so I'… #Patch Set 7 : chrome elves fixed my problem #Patch Set 8 : Merge with most recent again #
Total comments: 4
Patch Set 9 : Updated comments #
Total comments: 1
Patch Set 10 : Fixed a build break and set solid to false on empty picturemap #Patch Set 11 : Fix a mac specific build break #
Messages
Total messages: 39 (0 generated)
Hi all, since I've lost the ability to keep solid state in the SkPictureData, I'm using the opCount and AnalysisCanvas to track it. Please check it out, thanks!
https://codereview.chromium.org/494503002/diff/20001/cc/resources/picture.cc File cc/resources/picture.cc (right): https://codereview.chromium.org/494503002/diff/20001/cc/resources/picture.cc#... cc/resources/picture.cc:440: return bounds.width() * bounds.height(); If you're not using this, then let's just make this a void function. https://codereview.chromium.org/494503002/diff/20001/cc/resources/picture.h File cc/resources/picture.h (right): https://codereview.chromium.org/494503002/diff/20001/cc/resources/picture.h#n... cc/resources/picture.h:77: int approximateOpCount() const; This isn't named in cc style. Either approximate_op_count or GetApproximateOpCount would work, I think. https://codereview.chromium.org/494503002/diff/20001/cc/resources/picture.h#n... cc/resources/picture.h:88: int AnalysisRaster(SkCanvas* canvas, SkDrawPictureCallback* callback); nit: RasterForAnalysis or Analyze https://codereview.chromium.org/494503002/diff/20001/cc/resources/picture_pil... File cc/resources/picture_pile.cc (right): https://codereview.chromium.org/494503002/diff/20001/cc/resources/picture_pil... cc/resources/picture_pile.cc:151: is_solid_color_(true), I'd prefer default to be false, since.. paranoia. https://codereview.chromium.org/494503002/diff/20001/cc/resources/picture_pil... cc/resources/picture_pile.cc:423: if (it == picture_map_.end() || NULL == picture) { You can't get the picture from |it|, and then check if it's pointing to the end. Either the check is always true, or you can't dereference an iterator pointing to end. Also, NULL checks are usually written as !picture in cc. Also.. can we even have null pictures? :) I think this should be something along the lines of the following: if (picture_map_.empty()) { is_solid_color_ = true; solid_color_ = SK_ColorTRANSPARENT; return; } Picture* picture = NULL; for (... it = picture_map_.begin(); it != ...; ++it) { if (!picture) picture = it->second.GetPicture(); if (picture != it->second.GetPicture()) return; } // Analyze https://codereview.chromium.org/494503002/diff/20001/cc/resources/picture_pil... cc/resources/picture_pile.cc:431: if (it->second.GetPicture() != picture) { nit: braces optional
Thanks, good finds! https://codereview.chromium.org/494503002/diff/20001/cc/resources/picture.cc File cc/resources/picture.cc (right): https://codereview.chromium.org/494503002/diff/20001/cc/resources/picture.cc#... cc/resources/picture.cc:440: return bounds.width() * bounds.height(); On 2014/08/19 23:55:37, vmpstr wrote: > If you're not using this, then let's just make this a void function. ok, I also removed the callback, also not using that. https://codereview.chromium.org/494503002/diff/20001/cc/resources/picture.h File cc/resources/picture.h (right): https://codereview.chromium.org/494503002/diff/20001/cc/resources/picture.h#n... cc/resources/picture.h:77: int approximateOpCount() const; On 2014/08/19 23:55:37, vmpstr wrote: > This isn't named in cc style. Either approximate_op_count or > GetApproximateOpCount would work, I think. Acknowledged. https://codereview.chromium.org/494503002/diff/20001/cc/resources/picture.h#n... cc/resources/picture.h:88: int AnalysisRaster(SkCanvas* canvas, SkDrawPictureCallback* callback); On 2014/08/19 23:55:37, vmpstr wrote: > nit: RasterForAnalysis or Analyze Acknowledged. https://codereview.chromium.org/494503002/diff/20001/cc/resources/picture_pil... File cc/resources/picture_pile.cc (right): https://codereview.chromium.org/494503002/diff/20001/cc/resources/picture_pil... cc/resources/picture_pile.cc:151: is_solid_color_(true), On 2014/08/19 23:55:37, vmpstr wrote: > I'd prefer default to be false, since.. paranoia. Not sure, my thinking was that an uninit'd picture pile is the same as a transparent solid layer. No? I'm treating DetermineIfSolidColor with the same logic. https://codereview.chromium.org/494503002/diff/20001/cc/resources/picture_pil... cc/resources/picture_pile.cc:423: if (it == picture_map_.end() || NULL == picture) { On 2014/08/19 23:55:37, vmpstr wrote: > You can't get the picture from |it|, and then check if it's pointing to the end. > Either the check is always true, or you can't dereference an iterator pointing > to end. > > Also, NULL checks are usually written as !picture in cc. > > Also.. can we even have null pictures? :) > > I think this should be something along the lines of the following: > > if (picture_map_.empty()) { > is_solid_color_ = true; > solid_color_ = SK_ColorTRANSPARENT; > return; > } > > Picture* picture = NULL; > for (... it = picture_map_.begin(); it != ...; ++it) { > if (!picture) > picture = it->second.GetPicture(); > if (picture != it->second.GetPicture()) > return; > } > > // Analyze Woops, bad!~ I didn't use your code, I want to first check the opcount, that's O(1), and early out before iterating over the map.
https://codereview.chromium.org/494503002/diff/40001/cc/resources/picture.cc File cc/resources/picture.cc (right): https://codereview.chromium.org/494503002/diff/40001/cc/resources/picture.cc#... cc/resources/picture.cc:435: picture_->draw(canvas, NULL); Make a comment here please explaining why we don't use a callback (if a DCHECK for op count here can be put in, that would be nice as well; but, if it's going to have 2 hard coded constants that are the same then don't do it) https://codereview.chromium.org/494503002/diff/40001/cc/resources/picture_pil... File cc/resources/picture_pile.cc (right): https://codereview.chromium.org/494503002/diff/40001/cc/resources/picture_pil... cc/resources/picture_pile.cc:420: // determine if the entire picture map is pointing to the same image style nit: Comments should be proper sentences (capitalization and a period at the end) https://codereview.chromium.org/494503002/diff/40001/cc/resources/picture_pil... cc/resources/picture_pile.cc:421: if (picture_map_.empty()) { We talked briefly about this yesterday: Is it possible that the picture_map_ is empty? If not, this should be a DCHECK. I'm OK with handling it as well, however. Up to you. https://codereview.chromium.org/494503002/diff/40001/cc/resources/picture_pil... cc/resources/picture_pile.cc:426: const int kOpCountThatIsOkToAnalyze = 10; // TODO(hendrikw) good value? Looks like a good enough value. Can you put this in anonymous namespace near the top of the file, please? Also as an FYI, typically we put TODOs above the line :) https://codereview.chromium.org/494503002/diff/40001/cc/resources/picture_pil... cc/resources/picture_pile.cc:436: while (++it != picture_map_.end()) { I'd still prefer a for loop here: for (++it; it != picture_map_.end(); ++it) { ... } https://codereview.chromium.org/494503002/diff/40001/cc/resources/picture_pil... File cc/resources/picture_pile_unittest.cc (right): https://codereview.chromium.org/494503002/diff/40001/cc/resources/picture_pil... cc/resources/picture_pile_unittest.cc:1049: client_.add_draw_rect(tiling_rect(), paint); I only count 8 draw commands, the limit is 10. Did I miscount or is there some other commands in there?
The thread checker stuff seems pretty dodgy to me. Can we just assume these days that we always have playback_ and that this is threadsafe (and in some other patch remove all the thread checking stuff)?
On 2014/08/20 17:47:39, enne wrote: > The thread checker stuff seems pretty dodgy to me. Can we just assume these > days that we always have playback_ and that this is threadsafe (and in some > other patch remove all the thread checking stuff)? I don't know about always having playback_ but please make sure we keep the thread checking for pictures until cloning is no longer necessary. I've seen a number of cases where these checks have caught critical bugs that are very hard to diagnose based on crash reports so I'd like to keep them until cloning can be removed.
Ok, then if we still need the thread checker code for now, this current analysis without any check seems wrong. https://codereview.chromium.org/494503002/diff/40001/cc/resources/picture.h File cc/resources/picture.h (right): https://codereview.chromium.org/494503002/diff/40001/cc/resources/picture.h#n... cc/resources/picture.h:77: int GetApproximateOpCount() const; style nit: No need for "get" in a function name.
On 2014/08/20 18:19:26, enne wrote: > Ok, then if we still need the thread checker code for now, this current analysis > without any check seems wrong. > > https://codereview.chromium.org/494503002/diff/40001/cc/resources/picture.h > File cc/resources/picture.h (right): > > https://codereview.chromium.org/494503002/diff/40001/cc/resources/picture.h#n... > cc/resources/picture.h:77: int GetApproximateOpCount() const; > style nit: No need for "get" in a function name. Currently, this code is safe because the analysis is done before we synchronize with the other thread (i.e. the other thread doesn't have access to the picture), but this code could be a problem if others try to call RasterForAnalysis on the wrong thread. Unfortunately, I don't see away around it. My second attempt was to move this to the impl side and delay the IsSolidColor until we needed it, but when software rasterizing it was called from a third thread and I ran into the same issue again. I could keep poking at it, maybe there's a function in PicturePileImpl that I could initialize the solid color where it's always on the right thread? Seems messy though.
On 2014/08/20 19:29:08, Hendrik wrote: > On 2014/08/20 18:19:26, enne wrote: > > Ok, then if we still need the thread checker code for now, this current > analysis > > without any check seems wrong. > > > > https://codereview.chromium.org/494503002/diff/40001/cc/resources/picture.h > > File cc/resources/picture.h (right): > > > > > https://codereview.chromium.org/494503002/diff/40001/cc/resources/picture.h#n... > > cc/resources/picture.h:77: int GetApproximateOpCount() const; > > style nit: No need for "get" in a function name. > > Currently, this code is safe because the analysis is done before we synchronize > with the > other thread (i.e. the other thread doesn't have access to the picture), but > this code > could be a problem if others try to call RasterForAnalysis on the wrong thread. > > Unfortunately, I don't see away around it. My second attempt was to move this > to the > impl side and delay the IsSolidColor until we needed it, but when software > rasterizing > it was called from a third thread and I ran into the same issue again. > > I could keep poking at it, maybe there's a function in PicturePileImpl that I > could > initialize the solid color where it's always on the right thread? Seems messy > though. Can we either somehow check that this picture's thread checker was never initialized (ie, it hasn't be rasterized) or maybe add a second thread checker for analysis specifically? Not sure either one of those is a clean solution though
It seems like the right place to do this is on the main thread and I agree that the code as written is safe; I just don't like having an exposed function that has no safety check on it.
On 2014/08/20 19:43:22, vmpstr wrote: > On 2014/08/20 19:29:08, Hendrik wrote: > > On 2014/08/20 18:19:26, enne wrote: > > > Ok, then if we still need the thread checker code for now, this current > > analysis > > > without any check seems wrong. > > > > > > https://codereview.chromium.org/494503002/diff/40001/cc/resources/picture.h > > > File cc/resources/picture.h (right): > > > > > > > > > https://codereview.chromium.org/494503002/diff/40001/cc/resources/picture.h#n... > > > cc/resources/picture.h:77: int GetApproximateOpCount() const; > > > style nit: No need for "get" in a function name. > > > > Currently, this code is safe because the analysis is done before we > synchronize > > with the > > other thread (i.e. the other thread doesn't have access to the picture), but > > this code > > could be a problem if others try to call RasterForAnalysis on the wrong > thread. > > > > Unfortunately, I don't see away around it. My second attempt was to move this > > to the > > impl side and delay the IsSolidColor until we needed it, but when software > > rasterizing > > it was called from a third thread and I ran into the same issue again. > > > > I could keep poking at it, maybe there's a function in PicturePileImpl that I > > could > > initialize the solid color where it's always on the right thread? Seems messy > > though. > > Can we either somehow check that this picture's thread checker was never > initialized (ie, it hasn't be rasterized) or maybe add a second thread checker > for analysis specifically? Not sure either one of those is a clean solution > though A second thread checker wouldn't work, the data, once synchronized, should always be accessed from the raster thread.
It sounds from 100 feet away that you just need to Reset the thread checker at some correct time. On Wed, Aug 20, 2014 at 3:43 PM, <vmpstr@chromium.org> wrote: > On 2014/08/20 19:29:08, Hendrik wrote: > >> On 2014/08/20 18:19:26, enne wrote: >> > Ok, then if we still need the thread checker code for now, this current >> analysis >> > without any check seems wrong. >> > >> > https://codereview.chromium.org/494503002/diff/40001/cc/ >> resources/picture.h >> > File cc/resources/picture.h (right): >> > >> > >> > > https://codereview.chromium.org/494503002/diff/40001/cc/ > resources/picture.h#newcode77 > >> > cc/resources/picture.h:77: int GetApproximateOpCount() const; >> > style nit: No need for "get" in a function name. >> > > Currently, this code is safe because the analysis is done before we >> > synchronize > >> with the >> other thread (i.e. the other thread doesn't have access to the picture), >> but >> this code >> could be a problem if others try to call RasterForAnalysis on the wrong >> > thread. > > Unfortunately, I don't see away around it. My second attempt was to move >> this >> to the >> impl side and delay the IsSolidColor until we needed it, but when software >> rasterizing >> it was called from a third thread and I ran into the same issue again. >> > > I could keep poking at it, maybe there's a function in PicturePileImpl >> that I >> could >> initialize the solid color where it's always on the right thread? Seems >> messy >> though. >> > > Can we either somehow check that this picture's thread checker was never > initialized (ie, it hasn't be rasterized) or maybe add a second thread > checker > for analysis specifically? Not sure either one of those is a clean solution > though > > https://codereview.chromium.org/494503002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2014/08/20 19:47:56, danakj wrote: > It sounds from 100 feet away that you just need to Reset the thread checker > at some correct time. > The thread checker doesn't have reset functionality, which I could add, but then we would then need to expose it from the picture, which I could add, but then anyone has the ability to circumvent this, which is ok, but all of this won't be needed when we switch to the playback_ system, which makes me sad.
On Wed, Aug 20, 2014 at 3:57 PM, <hendrikw@chromium.org> wrote: > On 2014/08/20 19:47:56, danakj wrote: > >> It sounds from 100 feet away that you just need to Reset the thread >> checker >> at some correct time. >> > > > The thread checker doesn't have reset functionality, which I could add, > but then > we would then need to expose it from the picture, which I could add, but > then > anyone has the ability to circumvent this, which is ok, but all of this > won't be > needed when we switch to the playback_ system, which makes me sad. > Does DetachFromThread() not do what you want? See NonThreadSafeImpl::DetachFromThread comment. > > > > https://codereview.chromium.org/494503002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2014/08/20 19:57:35, Hendrik wrote: > On 2014/08/20 19:47:56, danakj wrote: > > It sounds from 100 feet away that you just need to Reset the thread checker > > at some correct time. > > > > The thread checker doesn't have reset functionality, which I could add, but then > we would then need to expose it from the picture, which I could add, but then > anyone has the ability to circumvent this, which is ok, but all of this won't be > needed when we switch to the playback_ system, which makes me sad. Looks like it does have a reset function, DetachFromThread. I'll add the code to reset it :(
Fix for review comments - next change will do the reset thread https://codereview.chromium.org/494503002/diff/40001/cc/resources/picture.cc File cc/resources/picture.cc (right): https://codereview.chromium.org/494503002/diff/40001/cc/resources/picture.cc#... cc/resources/picture.cc:435: picture_->draw(canvas, NULL); On 2014/08/20 16:12:00, vmpstr wrote: > Make a comment here please explaining why we don't use a callback (if a DCHECK > for op count here can be put in, that would be nice as well; but, if it's going > to have 2 hard coded constants that are the same then don't do it) Acknowledged. https://codereview.chromium.org/494503002/diff/40001/cc/resources/picture.h File cc/resources/picture.h (right): https://codereview.chromium.org/494503002/diff/40001/cc/resources/picture.h#n... cc/resources/picture.h:77: int GetApproximateOpCount() const; On 2014/08/20 18:19:26, enne wrote: > style nit: No need for "get" in a function name. vmpstr suggested the name :) I think Get improves readabilty (and in some cases reduces ambiguity), but I've removed it anyway, in this case it doesn't matter https://codereview.chromium.org/494503002/diff/40001/cc/resources/picture_pil... File cc/resources/picture_pile.cc (right): https://codereview.chromium.org/494503002/diff/40001/cc/resources/picture_pil... cc/resources/picture_pile.cc:420: // determine if the entire picture map is pointing to the same image On 2014/08/20 16:12:00, vmpstr wrote: > style nit: Comments should be proper sentences (capitalization and a period at > the end) Done. https://codereview.chromium.org/494503002/diff/40001/cc/resources/picture_pil... cc/resources/picture_pile.cc:421: if (picture_map_.empty()) { On 2014/08/20 16:12:00, vmpstr wrote: > We talked briefly about this yesterday: Is it possible that the picture_map_ is > empty? If not, this should be a DCHECK. I'm OK with handling it as well, > however. Up to you. I think there are situations where the map could be empty. https://codereview.chromium.org/494503002/diff/40001/cc/resources/picture_pil... cc/resources/picture_pile.cc:426: const int kOpCountThatIsOkToAnalyze = 10; // TODO(hendrikw) good value? On 2014/08/20 16:12:00, vmpstr wrote: > Looks like a good enough value. Can you put this in anonymous namespace near the > top of the file, please? Also as an FYI, typically we put TODOs above the line > :) Done. https://codereview.chromium.org/494503002/diff/40001/cc/resources/picture_pil... cc/resources/picture_pile.cc:436: while (++it != picture_map_.end()) { On 2014/08/20 16:12:00, vmpstr wrote: > I'd still prefer a for loop here: > > for (++it; it != picture_map_.end(); ++it) { > ... > } Done. https://codereview.chromium.org/494503002/diff/40001/cc/resources/picture_pil... File cc/resources/picture_pile_unittest.cc (right): https://codereview.chromium.org/494503002/diff/40001/cc/resources/picture_pil... cc/resources/picture_pile_unittest.cc:1049: client_.add_draw_rect(tiling_rect(), paint); On 2014/08/20 16:12:00, vmpstr wrote: > I only count 8 draw commands, the limit is 10. Did I miscount or is there some > other commands in there? I've added a comment.
On Wed, Aug 20, 2014 at 4:42 PM, <hendrikw@chromium.org> wrote: > On 2014/08/20 19:57:35, Hendrik wrote: > >> On 2014/08/20 19:47:56, danakj wrote: >> > It sounds from 100 feet away that you just need to Reset the thread >> checker >> > at some correct time. >> > >> > > The thread checker doesn't have reset functionality, which I could add, >> but >> > then > >> we would then need to expose it from the picture, which I could add, but >> then >> anyone has the ability to circumvent this, which is ok, but all of this >> won't >> > be > >> needed when we switch to the playback_ system, which makes me sad. >> > > Looks like it does have a reset function, DetachFromThread. I'll add the > code > to reset it :( > Maybe that's not the right thing here, but I want to point out that it exists. I'll let enne/vmpstr decide if it's right. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Here's the ResetThreadChecker approach, I'll revert it if vmpstr and enne don't like it.
I found a case where we have null pictures in the map. Fixed the code to address this.
https://codereview.chromium.org/362073002 removes thread checker entirely. Can you rebase on top of that? On 2014/08/20 at 21:54:03, hendrikw wrote: > I found a case where we have null pictures in the map. Fixed the code to address this. When does this happen?
For realz? My problem has been magically solved! Rebased and removed the ResetThreadChecker function
On 2014/08/25 17:21:33, enne wrote: > On 2014/08/20 at 21:54:03, hendrikw wrote: > > I found a case where we have null pictures in the map. Fixed the code to > address this. > > When does this happen? I ran into it while opening the verge. The tile map had 144 tiles, most of which were solid, many (including the first one) which was nullptr. Once I finish compiling the latest, I'll figure out how they get into the map. Looking at the current code, it shouldn't be possible.
On 2014/08/25 21:10:36, Hendrik wrote:
> On 2014/08/25 17:21:33, enne wrote:
> > On 2014/08/20 at 21:54:03, hendrikw wrote:
> > > I found a case where we have null pictures in the map. Fixed the code to
> > address this.
> >
> > When does this happen?
>
> I ran into it while opening the verge. The tile map had 144 tiles, most of
> which were solid, many (including the first one) which was nullptr. Once I
> finish compiling the latest, I'll figure out how they get into the map.
Looking
> at the current code, it shouldn't be possible.
The map is being accessed via the [] operator on line 426
for (TilingData::Iterator it(&tiling_, interest_rect, include_borders); it;
++it) {
const PictureMapKey& key = it.index();
DCHECK(picture_map_.find(key) != picture_map_.end());
* PictureInfo& info = picture_map_[key];
gfx::Rect rect = PaddedRect(key);
This is where the nullptr pictures are added.
NeedsRecording and the else case both only do something if the picture is null.
* if (info.NeedsRecording(frame_number, distance_to_visible)) {
...
* } else if (!info.GetPicture()) {
...
}
But if we expect that null pictures shouldn't exist after we're out of
UpdateAndExpandInvalidation, then we should be doing a find() and change the
above code to work with that.
Ok, now I remember what this is supposed to do. The map can have null pictures because we may not have a recording but still want to keep track of the invalidation info. Sorry; having a null picture is totally legitimate. I think what you're doing for it is reasonable. https://codereview.chromium.org/494503002/diff/140001/cc/resources/picture_pi... File cc/resources/picture_pile.cc (right): https://codereview.chromium.org/494503002/diff/140001/cc/resources/picture_pi... cc/resources/picture_pile.cc:533: is_solid_color_ = true; // No pictures? Then we're solid 100% transparent. Is this true? I think the map being empty is maybe only possible if the bounds are empty. https://codereview.chromium.org/494503002/diff/140001/cc/resources/picture_pi... cc/resources/picture_pile.cc:540: // There are situations where the first picture is NULL, we'll assume not "There are situations where the first picture is NULL" => "Missing recordings due to frequent invalidations or being too far away from the interest rect"
So, what do we do when we try to rasterize a null picture? Do we ever? I guess I'll debug this too. I should make sure solid color detection works the same as rasterization. https://codereview.chromium.org/494503002/diff/140001/cc/resources/picture_pi... File cc/resources/picture_pile.cc (right): https://codereview.chromium.org/494503002/diff/140001/cc/resources/picture_pi... cc/resources/picture_pile.cc:533: is_solid_color_ = true; // No pictures? Then we're solid 100% transparent. On 2014/08/25 23:21:07, enne wrote: > Is this true? I think the map being empty is maybe only possible if the bounds > are empty. I don't think we should ever rasterize a picture_pile with an empty map, so it doesn't really matter what we set it to. I tried a DCHECK(!picture_map_.empty()) in PicturePileImpl::RasterForAnalysis and went to a bunch of sites, never hit. I've changed the comment, my preference is to still set it to true, but I'm fine with false too. I could add the DCHECK to the final code to backup my assumption. https://codereview.chromium.org/494503002/diff/140001/cc/resources/picture_pi... cc/resources/picture_pile.cc:540: // There are situations where the first picture is NULL, we'll assume not On 2014/08/25 23:21:07, enne wrote: > "There are situations where the first picture is NULL" => "Missing recordings > due to frequent invalidations or being too far away from the interest rect" Done.
lgtm On 2014/08/26 at 00:12:38, hendrikw wrote: > So, what do we do when we try to rasterize a null picture? Do we ever? > > I guess I'll debug this too. I should make sure solid color detection works the same as rasterization. No need. These tiles just don't get created, via checking PicturePileBase::CanRaster first. So rasterization never has to deal with missing pictures. https://codereview.chromium.org/494503002/diff/160001/cc/resources/picture_pi... File cc/resources/picture_pile.cc (right): https://codereview.chromium.org/494503002/diff/160001/cc/resources/picture_pi... cc/resources/picture_pile.cc:533: // if we have an empty map, the picture pile will never be rasterized and I'd just early out without a comment here. I think it doesn't matter what gets picked.
The CQ bit was checked by hendrikw@chromium.org
The CQ bit was unchecked by hendrikw@chromium.org
Fixed a build break and set solid to false on empty picturemap as Enne suggested. Thanks!
The CQ bit was checked by hendrikw@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hendrikw@chromium.org/494503002/180001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_clang_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...) android_dbg_tests_recipe on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...) linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_swarming on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_compile_dbg on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
Mac build break fix :(
The CQ bit was checked by hendrikw@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hendrikw@chromium.org/494503002/200001
Message was sent while issue was closed.
Committed patchset #11 (200001) as d90354d437ce1680b9acbfbe8972034dde2ddfe9
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/c60f5fbb953fb80e7f36cb7885e6f6c8b8494439 Cr-Commit-Position: refs/heads/master@{#292044} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
