Bug 13759 - REGRESSION (r20182-r20184): Incorrect rendering of multipart images
Summary: REGRESSION (r20182-r20184): Incorrect rendering of multipart images
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Images (show other bugs)
Version: 523.x (Safari 3)
Hardware: Macintosh OS X 10.4
: P1 Major
Assignee: Nobody
URL:
Keywords: HasReduction, InRadar, Regression
: 13642 (view as bug list)
Depends on:
Blocks:
 
Reported: 2007-05-17 05:33 PDT by mitz
Modified: 2014-12-08 23:58 PST (History)
5 users (show)

See Also:


Attachments
HTTP test in patch form (20.02 KB, patch)
2007-05-17 05:33 PDT, mitz
no flags Details | Formatted Diff | Diff
Band-aid fix (894 bytes, patch)
2007-05-17 08:38 PDT, mitz
no flags Details | Formatted Diff | Diff
Copy the current part's data before it is overwritten by the next part (54.31 KB, patch)
2007-05-17 10:02 PDT, mitz
beidson: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description mitz 2007-05-17 05:33:04 PDT
[This is possibly a duplicate of bug 13642]

To reproduce this bug, apply the attached patch to LayoutTests and execute 'run-webkit-httpd'. Then in WebKit go to <http://127.0.0.1:8000/multipart/image-decoding.html>.

Expected result: The image should cycle between a blue square, a green square and Abe.

Actual result: The image cycles between a white square, a green square, and a grey line.

Notes: Firefox and shipping Safari render the images as expected. So far this doesn't seem to be a loader bug, as I have observed that ImageSource::setData() is called with the correct data.
Comment 1 mitz 2007-05-17 05:33:47 PDT
Created attachment 14589 [details]
HTTP test in patch form
Comment 3 mitz 2007-05-17 06:25:59 PDT
The problem is that ImageSource::setData() does not copy the data, and in the case of multipart images, the buffer becomes invalid by the time createFrameAtIndex() is called.
Comment 4 mitz 2007-05-17 08:38:39 PDT
Created attachment 14593 [details]
Band-aid fix

The way the loader currently works, only when a new part arrives, the previous part is delivered to the client. This means that at time t, Safari always shows you part t-1. This is in fact the behavior of shipping Safari as well. You can see it by changing the timeouts in the test case to make them uneven: change the sleep(1) after abe to sleep(4) and run the test. Notice how it is the green square, rather than abe, that is displayed for 4 seconds in each cycle. In contrast, in Firefox it is indeed abe that is displayed for 4 seconds.

This patch doesn't fix the above behavior, but accounting for it, it ensures that the client gets its own copy of the data so that it can use it after the resource data changes to contain (parts of) the next part.

I think it's worth applying this fix for now, until the aforementioned bug is addressed, so I will try to make a test to go with this patch.
Comment 5 mitz 2007-05-17 10:02:56 PDT
Created attachment 14595 [details]
Copy the current part's data before it is overwritten by the next part

Includes a pixel test. Also moves the last multipart test together with the new one into a new 'multipart' subdirectory where they share the multipart.php script.
Comment 6 Brady Eidson 2007-05-17 10:56:38 PDT
Comment on attachment 14595 [details]
Copy the current part's data before it is overwritten by the next part

I agree the larger bug here is we need to be displaying frame T, not T-1.

But I think this is a solid fix to land now.  r=me
Comment 7 Brady Eidson 2007-05-17 11:10:39 PDT
<rdar://problem/5210662>
Comment 8 Brady Eidson 2007-05-17 11:16:35 PDT
Landed in r21536
Comment 9 Brady Eidson 2007-05-17 11:55:17 PDT
*** Bug 13642 has been marked as a duplicate of this bug. ***
Comment 10 Peter 2007-05-18 01:58:48 PDT
Webkit does not crash anymore, webcam renders smoothly. Thanks for the effort! The address bar keeps showing the blue "loading" bar though. Is there a separate bug yet for this?
Comment 11 mitz 2007-05-18 02:06:46 PDT
(In reply to comment #10)
> The address bar keeps showing the blue "loading" bar though. Is there a
> separate bug yet for this?

Bug 13763.