{"push_id":657441401,"size":1,"distinct_size":1,"ref":"refs/heads/master","head":"cfbbde1acd8d1250c38323c4bfa818a7103073c7","before":"70afc4633700f439232ec751406d35f27536d19d","commits":[{"sha":"cfbbde1acd8d1250c38323c4bfa818a7103073c7","author":{"email":"af2833f195af96c34185f741f3f4394197a8590f@chromium.org","name":"pkasting@chromium.org"},"message":"Properly notify ImageFrame of pixel changes from the BMP/ICO decoders\n\nImageFrame::setPixelsChanged()/notifyBitmapIfPixelsChanged() were added by\nhttps://codereview.chromium.org/75303004 , which modified the other decoders to\ncall them.  Alpha asked if BMP and ICO needed to be modified, and the author\nreplied:\n\n\"I don't think we need any changes to the BMP and ICO decoders:\n\n\"It looks like BMP doesn't support progressive decoding or multiple frames.\nBMPImageReader::decodeBMP initializes the ImageFrame (which then has\ngenerationID==0). The function either completes decoding or it fails.\n\n\"ICO uses a BMP or a PNG decoder internally. For BMP we don't need to do\nanything. For PNG, the ICO decoder calls PNGImageDecoder::frameBufferAtIndex,\nwhich handles the change notification.\"\n\nWhile the part about PNG-in-ICO is correct, the part about BMP is not.  BMP does\nsupport progressive decoding, and decodeBMP() does not fail if the image cannot\nyet be decoded completely (in the sense of calling setFailed()).  Instead, like\nthe other image decoders, it decodes as much as possible and then returns\nwithout marking the frame complete.  (Hint to future authors: get the guy who\nactually wrote the decoder you're analyzing to review your analysis of it...)\n\nThe consequence of this is not clear to me because I don't understand what\nSkPixelRef generation IDs do.  I hypothesize that, for BMPs and BMP-in-ICOs, the\nresult is that a second decode after a first, incomplete decode will not always\nresult in everywhere using the updated pixel results.  Since I don't know,\nthough, I haven't filed a bug, and I don't know how to test this (manually or\notherwise).  Information welcome.\n\nThis change adds the appropriate bits to these decoders to do the notifications:\n* ICOImageDecoder::frameBufferAtIndex() now calls notifyBitmapIfPixelsChanged().\n  For PNG-in-ICO this is unnecessary as the nested\n  PNGImageDecoder::frameBufferAtIndex() call will have done this, but it's\n  harmless (will quickly no-op).\n* BMPImageDecoder::frameBufferAtIndex() also calls this.\n* Most importantly, BMPImageReader::decodeBMP() now calls setPixelsChanged()\n  when calls to ProcessRLEData() or processNonRLEData() result in decoding more\n  of the image.  Without this call, the above calls would do nothing.  This is\n  implemented by means of a function-local scoping class.  (I could have just\n  put the necessary conditionals in the code directly, but it would have\n  involved repeated code blocks, and I would have needed to rework some of the\n  existing code to have a shared exit point before which I could insert the\n  appropriate call.  Hopefully my solution is not too overdesigned.)\n\nIt would also be possible to call setPixelsChanged() directly from e.g. setI()\nand setRGBA(), but I'm concerned about the potential performance impact of doing\nthese calls (even cheap ones) on every pixel.  (In the original review, Alpha\nhad the same concerns about a similar such suggestion.)  Several other decoders\ncall this once per row, since that's architecturally convenient; here it was\njust as easy to do them \"almost optimally rarely\".\n\nTEST=None\nBUG=485527\n\nReview URL: https://codereview.chromium.org/1123363006\n\ngit-svn-id: svn://svn.chromium.org/blink/trunk@195107 bbb929c8-8fbe-4397-9dbb-9b2b20218538","distinct":true,"url":"https://api.github.com/repos/dstockwell/blink/commits/cfbbde1acd8d1250c38323c4bfa818a7103073c7"}]}