Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1737593003:
hack up get_images_from_skps (Closed)
Descriptionhack up get_images_from_skps
I couldn't get the version at head to give me any images,
so I decided to rewrite it. Does something like this work
for you?
BUG=skia:5010, skia:5005
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1737593003
Committed: https://skia.googlesource.com/skia/+/2d225e3a0f968e866409e5b300dc1e053ba4a7b7
Patch Set 1 #Patch Set 2 : md5 is probably a better idea #
Messages
Total messages: 25 (9 generated)
Description was changed from ========== hack up get_images_from_skps BUG=skia: ========== to ========== hack up get_images_from_skps BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
Description was changed from ========== hack up get_images_from_skps BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== hack up get_images_from_skps BUG=skia:5010 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
Description was changed from ========== hack up get_images_from_skps BUG=skia:5010 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== hack up get_images_from_skps I couldn't get the version at head to give me any images, so I decided to rewrite it. BUG=skia:5010 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
Description was changed from ========== hack up get_images_from_skps I couldn't get the version at head to give me any images, so I decided to rewrite it. BUG=skia:5010 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== hack up get_images_from_skps I couldn't get the version at head to give me any images, so I decided to rewrite it. Does something like this work for you? BUG=skia:5010 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
mtklein@chromium.org changed reviewers: + msarett@google.com
msarett@google.com changed reviewers: + scroggo@google.com
Yes the tool is broken on newer skps. Can you link skbug.com/5005? For a particular set of skps, this generates 2138 images, where the old tool (with my fix from https://codereview.chromium.org/1737543002/) outputs 4911? I'm hoping that this is an improvement because this tool removes duplicates (rather than an indication that we are missing some images)?
Description was changed from ========== hack up get_images_from_skps I couldn't get the version at head to give me any images, so I decided to rewrite it. Does something like this work for you? BUG=skia:5010 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== hack up get_images_from_skps I couldn't get the version at head to give me any images, so I decided to rewrite it. Does something like this work for you? BUG=skia:5010,skia:5005 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
On 2016/02/25 15:51:02, msarett wrote: > Yes the tool is broken on newer skps. Can you link skbug.com/5005? Done. > For a particular set of skps, this generates 2138 images, where the old tool > (with my fix from https://codereview.chromium.org/1737543002/) outputs 4911? > I'm hoping that this is an improvement because this tool removes duplicates > (rather than an indication that we are missing some images)? For the v671 set of SKPs, I get 2188 unique images. Which set are you looking at?
On 2016/02/25 15:51:02, msarett wrote: > Yes the tool is broken on newer skps. Can you link skbug.com/5005? > > For a particular set of skps, this generates 2138 images, where the old tool > (with my fix from https://codereview.chromium.org/1737543002/) outputs 4911? > I'm hoping that this is an improvement because this tool removes duplicates > (rather than an indication that we are missing some images)? FWIW, if I remove the dedup logic, this tool generates 15135 images... I was hoping to see 4911 :).
On 2016/02/25 15:54:41, mtklein wrote: > On 2016/02/25 15:51:02, msarett wrote: > > Yes the tool is broken on newer skps. Can you link skbug.com/5005? > Done. > > > > For a particular set of skps, this generates 2138 images, where the old tool > > (with my fix from https://codereview.chromium.org/1737543002/) outputs 4911? > > I'm hoping that this is an improvement because this tool removes duplicates > > (rather than an indication that we are missing some images)? > > For the v671 set of SKPs, I get 2188 unique images. Which set are you looking > at? gs://chromium-skia-gm/playback_675/skps/
On 2016/02/25 15:55:51, msarett wrote: > On 2016/02/25 15:51:02, msarett wrote: > > Yes the tool is broken on newer skps. Can you link skbug.com/5005? > > > > For a particular set of skps, this generates 2138 images, where the old tool > > (with my fix from https://codereview.chromium.org/1737543002/) outputs 4911? > > I'm hoping that this is an improvement because this tool removes duplicates > > (rather than an indication that we are missing some images)? > > FWIW, if I remove the dedup logic, this tool generates 15135 images... > I was hoping to see 4911 :). It's a little different than you think. This dedup logic isn't removing duplicate images in the same .skp... the SKPs usually do that already. It's removing duplicate uses of that same image with the .skp.
On 2016/02/25 15:56:59, msarett wrote: > On 2016/02/25 15:54:41, mtklein wrote: > > On 2016/02/25 15:51:02, msarett wrote: > > > Yes the tool is broken on newer skps. Can you link skbug.com/5005? > > Done. > > > > > > > For a particular set of skps, this generates 2138 images, where the old tool > > > (with my fix from https://codereview.chromium.org/1737543002/) outputs 4911? > > > > I'm hoping that this is an improvement because this tool removes duplicates > > > (rather than an indication that we are missing some images)? > > > > For the v671 set of SKPs, I get 2188 unique images. Which set are you > looking > > at? > > gs://chromium-skia-gm/playback_675/skps/ Gotcha, I also get 2138 with that set.
On 2016/02/25 16:02:55, mtklein wrote: > On 2016/02/25 15:56:59, msarett wrote: > > On 2016/02/25 15:54:41, mtklein wrote: > > > On 2016/02/25 15:51:02, msarett wrote: > > > > Yes the tool is broken on newer skps. Can you link skbug.com/5005? > > > Done. > > > > > > > > > > For a particular set of skps, this generates 2138 images, where the old > tool > > > > (with my fix from https://codereview.chromium.org/1737543002/) outputs > 4911? > > > > > > I'm hoping that this is an improvement because this tool removes > duplicates > > > > (rather than an indication that we are missing some images)? > > > > > > For the v671 set of SKPs, I get 2188 unique images. Which set are you > > looking > > > at? > > > > gs://chromium-skia-gm/playback_675/skps/ > > Gotcha, I also get 2138 with that set. I like this CL. Working is better than not working :). I would just like to understand what happened to the "missing" images (or verify that they are duplicates). I'm assuming that hash collisions would be rare. Let me mess around with deduping the other approach...
lgtm
On 2016/02/25 16:07:01, msarett wrote: > On 2016/02/25 16:02:55, mtklein wrote: > > On 2016/02/25 15:56:59, msarett wrote: > > > On 2016/02/25 15:54:41, mtklein wrote: > > > > On 2016/02/25 15:51:02, msarett wrote: > > > > > Yes the tool is broken on newer skps. Can you link skbug.com/5005? > > > > Done. > > > > > > > > > > > > > For a particular set of skps, this generates 2138 images, where the old > > tool > > > > > (with my fix from https://codereview.chromium.org/1737543002/) outputs > > 4911? > > > > > > > > I'm hoping that this is an improvement because this tool removes > > duplicates > > > > > (rather than an indication that we are missing some images)? > > > > > > > > For the v671 set of SKPs, I get 2188 unique images. Which set are you > > > looking > > > > at? > > > > > > gs://chromium-skia-gm/playback_675/skps/ > > > > Gotcha, I also get 2138 with that set. > > I like this CL. Working is better than not working :). I would just like > to understand what happened to the "missing" images (or verify that they > are duplicates). I'm assuming that hash collisions would be rare. Yes, rare enough that most other bugs are far more likely.
On 2016/02/25 16:15:48, mtklein wrote: > On 2016/02/25 16:07:01, msarett wrote: > > On 2016/02/25 16:02:55, mtklein wrote: > > > On 2016/02/25 15:56:59, msarett wrote: > > > > On 2016/02/25 15:54:41, mtklein wrote: > > > > > On 2016/02/25 15:51:02, msarett wrote: > > > > > > Yes the tool is broken on newer skps. Can you link skbug.com/5005? > > > > > Done. > > > > > > > > > > > > > > > > For a particular set of skps, this generates 2138 images, where the > old > > > tool > > > > > > (with my fix from https://codereview.chromium.org/1737543002/) outputs > > > 4911? > > > > > > > > > > I'm hoping that this is an improvement because this tool removes > > > duplicates > > > > > > (rather than an indication that we are missing some images)? > > > > > > > > > > For the v671 set of SKPs, I get 2188 unique images. Which set are you > > > > looking > > > > > at? > > > > > > > > gs://chromium-skia-gm/playback_675/skps/ > > > > > > Gotcha, I also get 2138 with that set. > > > > I like this CL. Working is better than not working :). I would just like > > to understand what happened to the "missing" images (or verify that they > > are duplicates). I'm assuming that hash collisions would be rare. > > Yes, rare enough that most other bugs are far more likely. As mentioned above this approach produces 2138 unique images. Deduping the other approach, there are 2158 unique images. I'm curious how we lost a few images, but I'm guessing it's not a big deal? lgtm
On 2016/02/25 16:49:02, msarett wrote: Let's track them down? What type are they, and what .skps do they come from?
On 2016/02/25 16:54:46, mtklein wrote: > On 2016/02/25 16:49:02, msarett wrote: > > Let's track them down? What type are they, and what .skps do they come from? tabl_gamedeksiam.skp produces 143 images using the old method and 133 using this one. I'll chat you one of the missing images.
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/1737593003/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1737593003/20001
Message was sent while issue was closed.
Description was changed from ========== hack up get_images_from_skps I couldn't get the version at head to give me any images, so I decided to rewrite it. Does something like this work for you? BUG=skia:5010,skia:5005 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== hack up get_images_from_skps I couldn't get the version at head to give me any images, so I decided to rewrite it. Does something like this work for you? BUG=skia:5010,skia:5005 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/2d225e3a0f968e866409e5b300dc1e053ba4a7b7 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://skia.googlesource.com/skia/+/2d225e3a0f968e866409e5b300dc1e053ba4a7b7 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
