Bug 92470 - Add more layout tests on image decoding
Summary: Add more layout tests on image decoding
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: Images (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 90375
  Show dependency treegraph
 
Reported: 2012-07-27 01:12 PDT by Kwang Yul Seo
Modified: 2013-03-21 18:02 PDT (History)
6 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Kwang Yul Seo 2012-07-27 01:12:14 PDT
Image decoding has been historically under-tested because there is no way to check if image decoding is done correctly. To write more layout tests on image decoding, we need a test API to query image decoding internals.
Comment 1 Hin-Chung Lam 2012-08-06 11:55:35 PDT
I have investigated testing on image decoding. I think there are several problems in this area:

1. No testing for partially loaded (and decoded) images.

When an image is loading from the network image is partially decoded and rendered. There is no test for this scenario. Pixel test for this is not possible today because DRT requires image to be loaded before dumping anything, hence a stalled network condition results in test timeout.

2. No test for image decoding implementation

This is probably best done by unit test. Expected behavior of ImageDecoder and ImageSource APIs should best be tested with unit test. For example frameHasAlphaAtIndex, frameOrentationAtIndex, frameDurationAtIndex.

3. Testing for operation sequence

Extending to asynchronous image decoding and parallel image decoding the sequence of operations is very important. This again should best be done with unit test.

I would like to hear comments on how we should better testing the existing code and extend into asynchronous decoding.
Comment 2 Hin-Chung Lam 2012-08-06 17:45:15 PDT
I still think
Comment 3 Hin-Chung Lam 2012-08-06 17:55:02 PDT
I still think we should have some unit tests for image decoding. I'm surprised to see a large of code under platform/image-decoders without low level functional testing. This is important to catch regression and crashes. Even if this is WebKit unit test instead of WebCore it will be very valuable. But before I stir up another unit test vs layout test discussion I would like to test this as best as I could with web platform first.

The first thing that is missing is the lack of pixel tests for partially loaded images. When an image is loaded from the network it is partially decoded and rendered with partial transparency.

We cannot test this today because DRT requires images to be loaded before dump the pixels. This forbids writing a test that simulate a stalled network. This seems like the first thing to work on. I'm picking this up.
Comment 4 Dongseong Hwang 2012-08-06 20:15:46 PDT
(In reply to comment #1)
> 2. No test for image decoding implementation
> 3. Testing for operation sequence

(In reply to comment #3)
> I still think we should have some unit tests for image decoding.

Before I read your comments, I thought about how to add a test API to query image decoding internals in DRT.
I am reluctant to expose image decoder's detail in DRT, because it seems there is a tight coupling between low level implementation detail of image decoding and high level web platform.
I think unit test is more suitable for testing sync and async image decoding, and we can use TestWebKitAPI to make image decoding unit tests.
If there are no other opinions about how to test sync and async image decoding, I will add unit tests that cover current behavior of ImageSource.
Comment 5 Hin-Chung Lam 2012-08-07 21:38:34 PDT
> Before I read your comments, I thought about how to add a test API to query image decoding internals in DRT.
> I am reluctant to expose image decoder's detail in DRT, because it seems there is a tight coupling between low level implementation detail of image decoding and high level web platform.
> I think unit test is more suitable for testing sync and async image decoding, and we can use TestWebKitAPI to make image decoding unit tests.
> If there are no other opinions about how to test sync and async image decoding, I will add unit tests that cover current behavior of ImageSource.

I would like to discuss my plan for layout tests:

1. Partially loaded image

DRT supports only fully loaded images, I would like to extend this to partially loaded images. See https://bugs.webkit.org/show_bug.cgi?id=93314 for progress. The problem with this is WebKit doesn't seem to rendering partial loaded and transparent loaded images. I'll report tomorrow to see if this is correct.

2. Enable asynchronous image decoding through DRT

Because of the native of asynchronous image decoding, a loaded <img> doesn't mean that it is rendered completely. We probably don't want to enable asynchronous image decoding by default for DRT otherwise there will be flakiness in pixel tests. We should enable asynchronous image decoding though Settings, see http://trac.webkit.org/browser/trunk/Source/WebCore/page/Settings.h and allow DRT to enable this.

3. Wait for image decoding to complete

Now with asynchronous image enabled in the test case, there should be a method like testRunner.waitUntilImagesDecoded() that exits DRT after images are all decoded.

4. Write <img> tests with async image decoding enabled and testRunner.waitUntilImagesDecoded()

We'll need to cover all basic image formats with asynchronous image decoding. Also cover the case with multiple images, images outside of viewport, etc.

In terms of unit test, I think it is important to verify the asynchronous behavior of ImageSource, and also its public methods. I think in general WebCore unit tests are discouraged and unit tests should be done in WebKit API level. However WebKit API is defined only on a platform, e.g. http://trac.webkit.org/browser/trunk/Source/WebKit/chromium/public/WebImageDecoder.h. I am fine testing it there but my concern is we test it only in one place. I would like to avoid having the same set of unit tests written in both chromium and qt. I am unsure if we can write one test file but runs on both qt and chromium. I propose we write one set of unit tests under http://trac.webkit.org/browser/trunk/Source/WebKit/chromium/tests.
Comment 6 Dongseong Hwang 2012-08-07 23:58:20 PDT
(In reply to comment #5)

Thank your for your explanation.

> 1. Partially loaded image
> 2. Enable asynchronous image decoding through DRT
> 3. Wait for image decoding to complete
> 4. Write <img> tests with async image decoding enabled and testRunner.waitUntilImagesDecoded()

All looks reasonable to me. I think your works can be applied to parallel image decoder also.

> unit tests should be done in WebKit API level.

Now I understand the policy of WebKit community.
Comment 7 Hin-Chung Lam 2012-08-08 17:03:54 PDT
(In reply to comment #6)
> (In reply to comment #5)
> 
> Thank your for your explanation.
> 
> > 1. Partially loaded image

I've created a test case that does this: https://bugs.webkit.org/show_bug.cgi?id=93171

We can expand this to other formats.

> > 2. Enable asynchronous image decoding through DRT
> > 3. Wait for image decoding to complete
> > 4. Write <img> tests with async image decoding enabled and testRunner.waitUntilImagesDecoded()
> 
> All looks reasonable to me. I think your works can be applied to parallel image decoder also.
> 
> > unit tests should be done in WebKit API level.
> 
> Now I understand the policy of WebKit community.

I'll start writing some unit tests for ImageSource using Chromium's WebKit API. We can then use that to test changes to ImageSource and ImageDecoder.
Comment 8 Hin-Chung Lam 2012-08-08 20:33:16 PDT
> 
> I'll start writing some unit tests for ImageSource using Chromium's WebKit API. We can then use that to test changes to ImageSource and ImageDecoder.

Now that I think this twice I think it's better to have a qt unit test as well. Unit test in Chromium can test the asynchronous API of ImageSource when we refactor it, but since Chromium won't be using parallel image decoder (we use it to defer image decoding instead) so it won't be able to test your parallel image decoder.