Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1178673007:
Allow reset of the SkPictureRecorder cull rect before endRecording. (Closed)
|
Created:
5 years, 6 months ago by Stephen Chennney Modified:
5 years, 5 months ago CC:
reviews_skia.org Base URL:
https://skia.googlesource.com/skia.git@master Target Ref:
refs/heads/master Project:
skia Visibility:
Public. |
DescriptionAllow reset of the SkPictureRecorder cull rect and other parameters during endRecording.
For some users of SkPictureRecorder, the cull rect is more efficiently
determined while drawing is in progress, rather than when recording starts.
The existing API requires the cull rect at start time, even though the
information is not used for any culling purpose until the end of recording.
This patch provides a means to reset the cull rect when recording ends,
allowing users to update the rect based on information learned during
drawing and for the new rect to be used as the culling bound. A valid
bound is still required on the beginRecording call because
it sizes the underlying canvas and sets the aspect ratio for any bounding
box hierarchy. The bounding box factory can also be specified and parameters
that control SkPicture creation.
R=mtklein, reed1
BUG=skia:3919
Committed: https://skia.googlesource.com/skia/+/eeff8bb8ffdd6e77823a23bad3d23e4f15057681
Patch Set 1 #Patch Set 2 : Switch to parameters in endRecording #Patch Set 3 : Remove recordFlags from new method #
Total comments: 1
Patch Set 4 : Removed bbhFactory #
Messages
Total messages: 31 (8 generated)
PTAL. Potential improvements: - make the aspectRatio for the BBH hierarchy reset also. - improve the test to verify that stuff outside the initial bound but inside the new rect still draws. I did not go with the initial proposal of an infinite initial canvas size and cull rect in the endRecording call. Firstly, it's hard to provide a back-compatible API that is not extremely confusing. Secondly, some code does allocate resources based on the initial rect, or so testing seems to indicate.
reed@google.com changed reviewers: + reed@google.com
reed@google.com changed required reviewers: + mtklein@google.com, reed@google.com
This isn't really how I'd like to leave the API here long-term, but I'm okay to
follow up myself after this as a first step hack to get you unblocked.
Long term I'd like to pass all of the cullRect, BBH factory, and flags to
endRecording() so that they're all consistent together. As written here today,
the BBH will be created with one cull rect then filled using another... this
happens to be ~fine given the current impl of SkRTreeBBH, but it's a bit weird
and fragile. The worst that happens for now is we build a slow R-tree, but it's
easy to envision ways to really break it unintentionally.
Do you see any reason we couldn't pass this new cull rect to endRecording()? If
we've got the information some time before endRecording(), we've got it at
endRecording(), no?
For now, I'm just talking about a hack as tiny as this before I take over:
SkPicture* endRecording(const SkRect& cull) {
fCull = cull;
return this->endRecording();
}
I'd like to grow an endRecording() like that into the canonical endRecording(),
then start stripping arguments from beginRecording().
On 2015/06/12 12:13:00, mtklein wrote:
> This isn't really how I'd like to leave the API here long-term, but I'm okay
to
> follow up myself after this as a first step hack to get you unblocked.
>
> Long term I'd like to pass all of the cullRect, BBH factory, and flags to
> endRecording() so that they're all consistent together. As written here
today,
> the BBH will be created with one cull rect then filled using another... this
> happens to be ~fine given the current impl of SkRTreeBBH, but it's a bit weird
> and fragile. The worst that happens for now is we build a slow R-tree, but
it's
> easy to envision ways to really break it unintentionally.
>
> Do you see any reason we couldn't pass this new cull rect to endRecording()?
If
> we've got the information some time before endRecording(), we've got it at
> endRecording(), no?
>
> For now, I'm just talking about a hack as tiny as this before I take over:
>
> SkPicture* endRecording(const SkRect& cull) {
> fCull = cull;
> return this->endRecording();
> }
>
> I'd like to grow an endRecording() like that into the canonical
endRecording(),
> then start stripping arguments from beginRecording().
I could do it that way. I initially avoided it because I thought it should be an
optional arg, but your way also essentially makes it optional. I'll go with that
and see what it looks like.
On 2015/06/12 17:31:52, Stephen Chenney wrote:
> On 2015/06/12 12:13:00, mtklein wrote:
> > This isn't really how I'd like to leave the API here long-term, but I'm okay
> to
> > follow up myself after this as a first step hack to get you unblocked.
> >
> > Long term I'd like to pass all of the cullRect, BBH factory, and flags to
> > endRecording() so that they're all consistent together. As written here
> today,
> > the BBH will be created with one cull rect then filled using another... this
> > happens to be ~fine given the current impl of SkRTreeBBH, but it's a bit
weird
> > and fragile. The worst that happens for now is we build a slow R-tree, but
> it's
> > easy to envision ways to really break it unintentionally.
> >
> > Do you see any reason we couldn't pass this new cull rect to endRecording()?
> If
> > we've got the information some time before endRecording(), we've got it at
> > endRecording(), no?
> >
> > For now, I'm just talking about a hack as tiny as this before I take over:
> >
> > SkPicture* endRecording(const SkRect& cull) {
> > fCull = cull;
> > return this->endRecording();
> > }
> >
> > I'd like to grow an endRecording() like that into the canonical
> endRecording(),
> > then start stripping arguments from beginRecording().
>
> I could do it that way. I initially avoided it because I thought it should be
an
> optional arg, but your way also essentially makes it optional. I'll go with
that
> and see what it looks like.
SGTM
The CQ bit was checked by schenney@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1178673007/20001
Note for Reviewers: The CQ is waiting for an approval. If you believe that the CL is not ready yet, or if you would like to L-G-T-M with comments then please uncheck the CQ checkbox. Waiting for LGTM from valid reviewer(s) till 2015-06-13 03:16 UTC
New version going through the CQ dry run now. It is nicer.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: All required reviewers (with asterisk prefixes) have not yet approved this CL. No LGTM from a valid reviewer yet. Please ask for an LGTM from a full Skia committer from https://skia.googlesource.com/skia/+/master/CQ_COMMITTERS
How do things operator if a BBHFactory was specified at the start and at the end (or flags were specified twice)? Are we making a contract promise that we *won't* ever build the BBH during recording?
On 2015/06/15 17:28:16, reed1 wrote: > How do things operator if a BBHFactory was specified at the start and at the end > (or flags were specified twice)? Are we making a contract promise that we > *won't* ever build the BBH during recording? Right now we would have to guarantee that the initial bbh will not be used before endRecording is invoked, because the code throws away any bbh it has from the beginRecording. That's if you use the new endRecording option, while the previous behavior is still available. I don't see a problem with documenting the behavior and transitioning to one API, or leaving both APIs and documenting what will happen if and when the RTree construction is changed. We also need to document the flags that are available, and when they apply. There's no documentation of those associated with beginRecording.
Where are we at on this? I think I've addressed the API concerns. To be useful we need it before Thursday's m45 branch cut.
Lets remove the factory/flag parameters from end(), since we only want to expose an overriding Cull for now.
On 2015/07/07 19:05:24, reed1 wrote: > Lets remove the factory/flag parameters from end(), since we only want to expose > an overriding Cull for now. SGTM. I'll update.
Patchset #3 (id:40001) has been deleted
On 2015/07/07 19:07:52, Stephen Chenney wrote: > On 2015/07/07 19:05:24, reed1 wrote: > > Lets remove the factory/flag parameters from end(), since we only want to > expose > > an overriding Cull for now. > > SGTM. I'll update. .. and now updated.
https://codereview.chromium.org/1178673007/diff/60001/include/core/SkPictureR... File include/core/SkPictureRecorder.h (right): https://codereview.chromium.org/1178673007/diff/60001/include/core/SkPictureR... include/core/SkPictureRecorder.h:89: SkBBHFactory* bbhFactory = NULL); Let's kill off bbhFactory too? The factory and cull rect being out of sync is something that's only technically a problem, but in an M45-M46 time frame we'll always be using RTree where it won't break anything, and is unlikely to have any effect at all (that aspect ratio thing is very minor).
The CQ bit was checked by schenney@chromium.org to run a CQ dry run
On 2015/07/07 19:18:16, mtklein wrote: > https://codereview.chromium.org/1178673007/diff/60001/include/core/SkPictureR... > File include/core/SkPictureRecorder.h (right): > > https://codereview.chromium.org/1178673007/diff/60001/include/core/SkPictureR... > include/core/SkPictureRecorder.h:89: SkBBHFactory* bbhFactory = NULL); > Let's kill off bbhFactory too? > > The factory and cull rect being out of sync is something that's only technically > a problem, but in an M45-M46 time frame we'll always be using RTree where it > won't break anything, and is unlikely to have any effect at all (that aspect > ratio thing is very minor). Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1178673007/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: All required reviewers (with asterisk prefixes) have not yet approved this CL. No LGTM from a valid reviewer yet. Please ask for an LGTM from a full Skia committer from https://skia.googlesource.com/skia/+/master/CQ_COMMITTERS
lgtm
lgtm
The CQ bit was checked by mtklein@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1178673007/80001
Message was sent while issue was closed.
Committed patchset #4 (id:80001) as https://skia.googlesource.com/skia/+/eeff8bb8ffdd6e77823a23bad3d23e4f15057681 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
