Closed Bug 1106138 Opened 8 years ago Closed 7 years ago

WebGL texture uploads: unpremultiplication is now handled as a separate pass (perf regression)

Categories

(Core :: Canvas: WebGL, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox42 --- fixed

People

(Reporter: bjacob, Assigned: kyle_fung)

References

(Depends on 1 open bug)

Details

(Whiteboard: [gfx-noted])

Attachments

(2 files, 1 obsolete file)

I was skimming the WebGL implementation to double check a WebGL book chapter that I'm writing.

Found this:

http://dxr.mozilla.org/mozilla-central/source/dom/canvas/WebGLContextGL.cpp#2681

nsresult
WebGLContext::SurfaceFromElementResultToImageSurface(nsLayoutUtils::SurfaceFromElementResult& res,
                                                     RefPtr<DataSourceSurface>& imageOut,
                                                     WebGLTexelFormat* format)
{
   *format = WebGLTexelFormat::None;

    if (!res.mSourceSurface)
        return NS_OK;
    RefPtr<DataSourceSurface> data = res.mSourceSurface->GetDataSurface();
    if (!data) {
        // SurfaceFromElement lied!
        return NS_OK;
    }

    if (!mPixelStorePremultiplyAlpha && res.mIsPremultiplied) {
        switch (data->GetFormat()) {
        case SurfaceFormat::B8G8R8X8:
            // No alpha, so de-facto premult'd.
            break;
        case SurfaceFormat::B8G8R8A8:
            data = gfxUtils::CreateUnpremultipliedDataSurface(data);
            break;
        default:
            MOZ_ASSERT(false, "Format unsupported.");
            break;
        }
    }

It makes no sense to call CreateUnpremultipliedDataSurface here: our WebGL texel conversions code is already able to handle the case of unpremultiplication, and it is better to let it handle that, as that allows doing everything in one pass --- fewer cycles and much better on memory bandwidth.

The above would be fine by itself for a quick and dirty first implementation, but the thing is, we used to have the better way implemented already!

Talking of better here - unpremultiplication sucks anyway, especially as it only imprecisely recovers the original image, so if we want the most correct results, we would have to implement it by re-decoding the image, similar to what we do for the no-colorspace-conversion case.
Whiteboard: [gfx-noted]
Assignee: nobody → kfung
Attached patch remove-early-premultiply.patch (obsolete) — Splinter Review
Let texel conversion code handle unpremultiplying the texture.
Attachment #8631371 - Flags: review?(jgilbert)
Made it so that when an unimplemented pack function is called, a crash will occur. Also implemented pack for RGBA32F which is needed to pass https://www.khronos.org/registry/webgl/conformance-suites/1.0.2/conformance/extensions/oes-texture-float-with-canvas.html

Try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=510a834e41c4
Attachment #8631372 - Flags: review?(jgilbert)
Attachment #8631371 - Flags: review?(jgilbert) → review+
Attachment #8631372 - Flags: review?(jgilbert) → review+
Keywords: checkin-needed
Needs rebasing.
Keywords: checkin-needed
Attachment #8631371 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/1ce930b301cb
https://hg.mozilla.org/mozilla-central/rev/571b4e0fdf53
Status: UNCONFIRMED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
See Also: → 1186111
Depends on: 1187731
You need to log in before you can comment on or make changes to this bug.