35287 2010-02-23 01:45:50 -0800 ImageSourceCG::frameIsCompleteAtIndex returns false for all frames until image has completed loading 2010-02-23 14:01:43 -0800 1 1 1 Unclassified WebKit Images 528+ (Nightly build) PC OS X 10.6 RESOLVED FIXED http://i.imgur.com/Zgc5p.gif P2 Normal --- 35115 1 mrowe webkit-unassigned ggaren mjs pkasting sam oldest_to_newest 192834 0 mrowe 2010-02-23 01:45:50 -0800 While looking in to bug 35115 I noticed that BitmapImage::frameIsCompleteAtIndex was always returning false until the complete image had loaded. Further analysis revealed that CGImageSourceGetStatusAtIndex seems to claim that all frames of an animated GIF are incomplete when using an incremental image source that has not yet received the complete data of the image. This certainly isn’t the behavior that the author of ImageSource::frameIsCompleteAtIndex (or any reasonable person) expects. I’m not sure if this behavior changed from Leopard to SnowLeopard, but I filed a bug against CGImageSourceGetStatusAtIndex expressing my displeasure at the misleading results that it gives (<rdar://problem/7679174>). We should attempt to work around <rdar://problem/7679174> and have frameIsCompleteAtIndex return a more useful answer. This will allow the fix from bug 35115 to be landed, which will fix an issue with the timing of the start of animations that are loaded from the cache (or a very quick network). 193014 1 49304 mrowe 2010-02-23 10:36:49 -0800 Created attachment 49304 Patch 193015 2 mrowe 2010-02-23 10:39:10 -0800 One user-visible impact of this change is that animated GIFs on Mac OS X and Windows no longer wait until they have completely loaded before they begin animating. 193023 3 49304 ggaren 2010-02-23 11:08:54 -0800 Comment on attachment 49304 Patch It looks like the two values we're interested in, when testing "frameStatus >= kCGImageStatusIncomplete", are: kCGImageStatusIncomplete = -1, kCGImageStatusComplete = 0 I think it would be better just to test them explicitly: "frameStatus == kCGImageStatusIncomplete || frameStatus == kCGImageStatusComplete". This has the benefit of documenting which values we're testing for without requiring the reader to flip to another file. Also, unless there's some explicit API contract somewhere, it's probably better not to assume that statuses >= kCGImageStatusIncomplete will always be "good" statuses. 193030 4 49304 darin 2010-02-23 11:30:27 -0800 Comment on attachment 49304 Patch > + // source (<rdar://problem/7679174>). We work around this by special-casing all frames except the We do a single space after periods. > + if (index < frameCount() - 1) > + return frameStatus >= kCGImageStatusIncomplete; Can this ever be called if the frameCount is 0? 193068 5 mrowe 2010-02-23 12:10:58 -0800 (In reply to comment #3) > (From update of attachment 49304 [details]) > It looks like the two values we're interested in, when testing "frameStatus >= > kCGImageStatusIncomplete", are: > kCGImageStatusIncomplete = -1, > kCGImageStatusComplete = 0 > > I think it would be better just to test them explicitly: "frameStatus == > kCGImageStatusIncomplete || frameStatus == kCGImageStatusComplete". > > This has the benefit of documenting which values we're testing for without > requiring the reader to flip to another file. Also, unless there's some > explicit API contract somewhere, it's probably better not to assume that > statuses >= kCGImageStatusIncomplete will always be "good" statuses. I was matching the manner in which the status values are tested elsewhere in ImageSourceCG.cpp: <http://trac.webkit.org/browser/trunk/WebCore/platform/graphics/cg/ImageSourceCG.cpp#L153>. 193071 6 mrowe 2010-02-23 12:14:46 -0800 (In reply to comment #4) > (From update of attachment 49304 [details]) > > + // source (<rdar://problem/7679174>). We work around this by special-casing all frames except the > > We do a single space after periods. Perhaps we should put this in the style guide ;-) > > + if (index < frameCount() - 1) > > + return frameStatus >= kCGImageStatusIncomplete; > > Can this ever be called if the frameCount is 0? It can’t be called with a frameCount of zero. I’ll add an assertion that frameCount is not zero since the code doesn’t correctly handle that case. 193118 7 mrowe 2010-02-23 14:01:43 -0800 Landed in r55169. 49304 2010-02-23 10:36:49 -0800 2010-02-23 11:30:27 -0800 Patch 0001-http-webkit.org-b-35287-ImageSourceCG-frameIsComplet.patch text/plain 4187 mrowe RnJvbSBjZjc5YzMzZmNiNWQ2NjgxN2NkZTQwNWJlZDE1NWExZGExMTkxODIzIE1vbiBTZXAgMTcg MDA6MDA6MDAgMjAwMQpGcm9tOiBNYXJrIFJvd2UgPG1yb3dlQGFwcGxlLmNvbT4KRGF0ZTogVHVl LCAyMyBGZWIgMjAxMCAxMDozNjowMCAtMDgwMApTdWJqZWN0OiBbUEFUQ0hdIDxodHRwOi8vd2Vi a2l0Lm9yZy9iLzM1Mjg3PiBJbWFnZVNvdXJjZUNHOjpmcmFtZUlzQ29tcGxldGVBdEluZGV4IHJl dHVybnMgZmFsc2UgZm9yIGFsbCBmcmFtZXMgdW50aWwgaW1hZ2UgaGFzIGNvbXBsZXRlZCBsb2Fk aW5nCgpSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KCkNHSW1hZ2VTb3VyY2VHZXRTdGF0dXNB dEluZGV4IGNsYWltcyB0aGF0IGFsbCBmcmFtZXMgb2YgYSBtdWx0aS1mcmFtZSBpbWFnZSBhcmUg aW5jb21wbGV0ZSB3aGVuIHdlJ3ZlIG5vdCB5ZXQgcmVjZWl2ZWQgdGhlCmNvbXBsZXRlIGRhdGEg Zm9yIGFuIGltYWdlIHRoYXQgaXMgdXNpbmcgYW4gaW5jcmVtZW50YWwgZGF0YSBzb3VyY2UgKDxy ZGFyOi8vcHJvYmxlbS83Njc5MTc0PikuIFdlIHdvcmsgYXJvdW5kIHRoaXMgYnkKc3BlY2lhbC1j YXNpbmcgYWxsIGZyYW1lcyBleGNlcHQgdGhlIGxhc3QgaW4gYW4gaW1hZ2UgYW5kIHRyZWF0aW5n IHRoZW0gYXMgY29tcGxldGUgaWYgdGhleSBhcmUgcHJlc2VudCBhbmQgcmVwb3J0ZWQgYXMKYmVp bmcgaW5jb21wbGV0ZS4gV2UgZG8gdGhpcyBvbiB0aGUgYXNzdW1wdGlvbiB0aGF0IGxvYWRpbmcg bmV3IGRhdGEgY2FuIG9ubHkgbW9kaWZ5IHRoZSBleGlzdGluZyBsYXN0IGZyYW1lIG9yIGFwcGVu ZCBuZXcKZnJhbWVzLiBUaGUgbGFzdCBmcmFtZSBpcyBvbmx5IHRyZWF0ZWQgYXMgYmVpbmcgY29t cGxldGUgaWYgdGhlIGltYWdlIHNvdXJjZSByZXBvcnRzIGl0IGFzIHN1Y2guIFRoaXMgZW5zdXJl cyB0aGF0IGl0IGlzCnRydWx5IHRoZSBsYXN0IGZyYW1lIG9mIHRoZSBpbWFnZSByYXRoZXIgdGhh biBqdXN0IHRoZSBsYXN0IHRoYXQgd2UgY3VycmVudGx5IGhhdmUgZGF0YSBmb3IuCgoqIHBsYXRm b3JtL2dyYXBoaWNzL2NnL0ltYWdlU291cmNlQ0cuY3BwOgooV2ViQ29yZTo6SW1hZ2VTb3VyY2U6 OmZyYW1lSXNDb21wbGV0ZUF0SW5kZXgpOgotLS0KIFdlYkNvcmUvQ2hhbmdlTG9nICAgICAgICAg ICAgICAgICAgICAgICAgICAgICAgfCAgIDE2ICsrKysrKysrKysrKysrKysKIFdlYkNvcmUvcGxh dGZvcm0vZ3JhcGhpY3MvY2cvSW1hZ2VTb3VyY2VDRy5jcHAgfCAgIDE1ICsrKysrKysrKysrKysr LQogMiBmaWxlcyBjaGFuZ2VkLCAzMCBpbnNlcnRpb25zKCspLCAxIGRlbGV0aW9ucygtKQoKZGlm ZiAtLWdpdCBhL1dlYkNvcmUvQ2hhbmdlTG9nIGIvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggY2Mx NzQwNy4uMmY5MGU5NCAxMDA2NDQKLS0tIGEvV2ViQ29yZS9DaGFuZ2VMb2cKKysrIGIvV2ViQ29y ZS9DaGFuZ2VMb2cKQEAgLTEsMyArMSwxOSBAQAorMjAxMC0wMi0yMyAgTWFyayBSb3dlICA8bXJv d2VAYXBwbGUuY29tPgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisg ICAgICAgIDxodHRwOi8vd2Via2l0Lm9yZy9iLzM1Mjg3PiBJbWFnZVNvdXJjZUNHOjpmcmFtZUlz Q29tcGxldGVBdEluZGV4IHJldHVybnMgZmFsc2UgZm9yIGFsbCBmcmFtZXMgdW50aWwgaW1hZ2Ug aGFzIGNvbXBsZXRlZCBsb2FkaW5nCisKKyAgICAgICAgQ0dJbWFnZVNvdXJjZUdldFN0YXR1c0F0 SW5kZXggY2xhaW1zIHRoYXQgYWxsIGZyYW1lcyBvZiBhIG11bHRpLWZyYW1lIGltYWdlIGFyZSBp bmNvbXBsZXRlIHdoZW4gd2UndmUgbm90IHlldCByZWNlaXZlZCB0aGUKKyAgICAgICAgY29tcGxl dGUgZGF0YSBmb3IgYW4gaW1hZ2UgdGhhdCBpcyB1c2luZyBhbiBpbmNyZW1lbnRhbCBkYXRhIHNv dXJjZSAoPHJkYXI6Ly9wcm9ibGVtLzc2NzkxNzQ+KS4gV2Ugd29yayBhcm91bmQgdGhpcyBieQor ICAgICAgICBzcGVjaWFsLWNhc2luZyBhbGwgZnJhbWVzIGV4Y2VwdCB0aGUgbGFzdCBpbiBhbiBp bWFnZSBhbmQgdHJlYXRpbmcgdGhlbSBhcyBjb21wbGV0ZSBpZiB0aGV5IGFyZSBwcmVzZW50IGFu ZCByZXBvcnRlZCBhcworICAgICAgICBiZWluZyBpbmNvbXBsZXRlLiBXZSBkbyB0aGlzIG9uIHRo ZSBhc3N1bXB0aW9uIHRoYXQgbG9hZGluZyBuZXcgZGF0YSBjYW4gb25seSBtb2RpZnkgdGhlIGV4 aXN0aW5nIGxhc3QgZnJhbWUgb3IgYXBwZW5kIG5ldworICAgICAgICBmcmFtZXMuIFRoZSBsYXN0 IGZyYW1lIGlzIG9ubHkgdHJlYXRlZCBhcyBiZWluZyBjb21wbGV0ZSBpZiB0aGUgaW1hZ2Ugc291 cmNlIHJlcG9ydHMgaXQgYXMgc3VjaC4gVGhpcyBlbnN1cmVzIHRoYXQgaXQgaXMKKyAgICAgICAg dHJ1bHkgdGhlIGxhc3QgZnJhbWUgb2YgdGhlIGltYWdlIHJhdGhlciB0aGFuIGp1c3QgdGhlIGxh c3QgdGhhdCB3ZSBjdXJyZW50bHkgaGF2ZSBkYXRhIGZvci4KKworICAgICAgICAqIHBsYXRmb3Jt L2dyYXBoaWNzL2NnL0ltYWdlU291cmNlQ0cuY3BwOgorICAgICAgICAoV2ViQ29yZTo6SW1hZ2VT b3VyY2U6OmZyYW1lSXNDb21wbGV0ZUF0SW5kZXgpOgorCiAyMDEwLTAyLTIyICBBbGV4ZXkgUHJv c2t1cnlha292ICA8YXBAYXBwbGUuY29tPgogCiAgICAgICAgIFJ1YmJlci1zdGFtcGVkIGJ5IEdl b2ZmIEdhcmVuLgpkaWZmIC0tZ2l0IGEvV2ViQ29yZS9wbGF0Zm9ybS9ncmFwaGljcy9jZy9JbWFn ZVNvdXJjZUNHLmNwcCBiL1dlYkNvcmUvcGxhdGZvcm0vZ3JhcGhpY3MvY2cvSW1hZ2VTb3VyY2VD Ry5jcHAKaW5kZXggMmIyYzZiMC4uYWFlODJjZCAxMDA2NDQKLS0tIGEvV2ViQ29yZS9wbGF0Zm9y bS9ncmFwaGljcy9jZy9JbWFnZVNvdXJjZUNHLmNwcAorKysgYi9XZWJDb3JlL3BsYXRmb3JtL2dy YXBoaWNzL2NnL0ltYWdlU291cmNlQ0cuY3BwCkBAIC0yMzQsNyArMjM0LDIwIEBAIENHSW1hZ2VS ZWYgSW1hZ2VTb3VyY2U6OmNyZWF0ZUZyYW1lQXRJbmRleChzaXplX3QgaW5kZXgpCiAKIGJvb2wg SW1hZ2VTb3VyY2U6OmZyYW1lSXNDb21wbGV0ZUF0SW5kZXgoc2l6ZV90IGluZGV4KQogewotICAg IHJldHVybiBDR0ltYWdlU291cmNlR2V0U3RhdHVzQXRJbmRleChtX2RlY29kZXIsIGluZGV4KSA9 PSBrQ0dJbWFnZVN0YXR1c0NvbXBsZXRlOworICAgIC8vIENHSW1hZ2VTb3VyY2VHZXRTdGF0dXNB dEluZGV4IGNsYWltcyB0aGF0IGFsbCBmcmFtZXMgb2YgYSBtdWx0aS1mcmFtZSBpbWFnZSBhcmUg aW5jb21wbGV0ZQorICAgIC8vIHdoZW4gd2UndmUgbm90IHlldCByZWNlaXZlZCB0aGUgY29tcGxl dGUgZGF0YSBmb3IgYW4gaW1hZ2UgdGhhdCBpcyB1c2luZyBhbiBpbmNyZW1lbnRhbCBkYXRhCisg ICAgLy8gc291cmNlICg8cmRhcjovL3Byb2JsZW0vNzY3OTE3ND4pLiAgV2Ugd29yayBhcm91bmQg dGhpcyBieSBzcGVjaWFsLWNhc2luZyBhbGwgZnJhbWVzIGV4Y2VwdCB0aGUKKyAgICAvLyBsYXN0 IGluIGFuIGltYWdlIGFuZCB0cmVhdGluZyB0aGVtIGFzIGNvbXBsZXRlIGlmIHRoZXkgYXJlIHBy ZXNlbnQgYW5kIHJlcG9ydGVkIGFzIGJlaW5nCisgICAgLy8gaW5jb21wbGV0ZS4gIFdlIGRvIHRo aXMgb24gdGhlIGFzc3VtcHRpb24gdGhhdCBsb2FkaW5nIG5ldyBkYXRhIGNhbiBvbmx5IG1vZGlm eSB0aGUgZXhpc3RpbmcgbGFzdAorICAgIC8vIGZyYW1lIG9yIGFwcGVuZCBuZXcgZnJhbWVzLiAg VGhlIGxhc3QgZnJhbWUgaXMgb25seSB0cmVhdGVkIGFzIGJlaW5nIGNvbXBsZXRlIGlmIHRoZSBp bWFnZSBzb3VyY2UKKyAgICAvLyByZXBvcnRzIGl0IGFzIHN1Y2guICBUaGlzIGVuc3VyZXMgdGhh dCBpdCBpcyB0cnVseSB0aGUgbGFzdCBmcmFtZSBvZiB0aGUgaW1hZ2UgcmF0aGVyIHRoYW4ganVz dAorICAgIC8vIHRoZSBsYXN0IHRoYXQgd2UgY3VycmVudGx5IGhhdmUgZGF0YSBmb3IuCisKKyAg ICBDR0ltYWdlU291cmNlU3RhdHVzIGZyYW1lU3RhdHVzID0gQ0dJbWFnZVNvdXJjZUdldFN0YXR1 c0F0SW5kZXgobV9kZWNvZGVyLCBpbmRleCk7CisgICAgaWYgKGluZGV4IDwgZnJhbWVDb3VudCgp IC0gMSkKKyAgICAgICAgcmV0dXJuIGZyYW1lU3RhdHVzID49IGtDR0ltYWdlU3RhdHVzSW5jb21w bGV0ZTsKKworICAgIHJldHVybiBmcmFtZVN0YXR1cyA9PSBrQ0dJbWFnZVN0YXR1c0NvbXBsZXRl OwogfQogCiBmbG9hdCBJbWFnZVNvdXJjZTo6ZnJhbWVEdXJhdGlvbkF0SW5kZXgoc2l6ZV90IGlu ZGV4KQotLSAKMS43LjAuMTcuZzdlNWViCgo=