Closed Bug 1250710 Opened 8 years ago Closed 8 years ago

Implement ReadPixels for PBOs (PIXEL_PACK_BUFFER)

Categories

(Core :: Graphics: CanvasWebGL, defect)

Product:
Core
Component:
Graphics: CanvasWebGL
Type:
defect
Priority:
Not set
Severity:
minor

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla50
Tracking Flags:
Tracking Status
firefox50 --- fixed

People

(Reporter: jgilbert, Assigned: jgilbert)

References

Details

(Whiteboard: [gfx-noted])

Attachments

(17 files, 4 obsolete files)

Bug 1250710 - Add webidl. -
(deleted), text/x-review-board-request
jgilbert
: review+
Details
Bug 1250710 - Add stub. -
(deleted), text/x-review-board-request
ethlin
: review+
Details
Bug 1250710 - Clean up formatting. -
(deleted), text/x-review-board-request
ethlin
: review+
Details
Bug 1250710 - Add PACK PBO support. -
(deleted), text/x-review-board-request
jrmuizel
: review+
Details
Bug 1250710 - Remove unused functions. -
(deleted), text/x-review-board-request
ethlin
: review+
Details
Bug 1250710 - Add a test. -
(deleted), text/x-review-board-request
ethlin
: review+
Details
Bug 1250710 - Unlock PIXEL_PACK_BUFFER. -
(deleted), text/x-review-board-request
ethlin
: review+
Details
Bug 1250710 - Update scoped pack/unpack helpers. -
(deleted), text/x-review-board-request
jrmuizel
: review+
Details
Bug 1250710 - Remove redundant IsCurrent checks. -
(deleted), text/x-review-board-request
ethlin
: review+
Details
Bug 1250710 - Remove packAlign save/restore from ScopedGLDrawState. -
(deleted), text/x-review-board-request
jrmuizel
: review+
Details
Bug 1250710 - Update usages of ScopedPackAlignment. -
(deleted), text/x-review-board-request
jrmuizel
: review+
Details
Bug 1250710 - Remove ScopedUnpackState due to unused. -
(deleted), text/x-review-board-request
ethlin
: review+
Details
Bug 1250710 - Mark specific InvalidEnum case. -
(deleted), text/x-review-board-request
ethlin
: review+
Details
Bug 1250710 - Workaround nvidia when stride is longer than the last row with PBOs. -
(deleted), text/x-review-board-request
jrmuizel
: review+
Details
Bug 1250710 - User of ReadPixel should clear PBO to 0. -
(deleted), text/x-review-board-request
ethlin
: review+
Details
Bug 1250710 - Save and restore state, since this is used by WebGL. -
(deleted), text/x-review-board-request
ethlin
: review+
Details
Bug 1250710 - ANGLE ES2 still requires HALF_FLOAT for ReadPixels, not HALF_FLOAT_OES. -
(deleted), text/x-review-board-request
ethlin
: review+
Details 
Here is my WIP:
https://github.com/jdashg/gecko-dev/tree/pbos

There is a test, but I don't test PixelStorei PACK_ params yet.
It compiles, but has an assert in ANGLE in the D3D11 backend. It looks like even in ANGLE master this path is not complete:
https://chromium.googlesource.com/angle/angle/+/8a854d68fc3166499f3dfb9031d7b4ccf7126b9e/src/libANGLE/renderer/d3d/d3d11/Buffer11.cpp#1262

Non-revisioned reference:
https://chromium.googlesource.com/angle/angle/+/master/src/libANGLE/renderer/d3d/d3d11/Buffer11.cpp#1262

Until this is fixed, we can't use the ANGLE backend if we want PBOs. If we want to move forward with implementation here, we need to disable PBOs for just ANGLE.
Whiteboard: [gfx-noted]
Is there an open bug for the ANGLE functionality, on our side, or Google side, that we can make block this one?
Blocks: webgl2
Flags: needinfo?(jgilbert)
(In reply to Milan Sreckovic [:milan] from comment #2)
> Is there an open bug for the ANGLE functionality, on our side, or Google
> side, that we can make block this one?

No. I can file one for our side.
Blocks: 1265852
Filed as bug 1265852.
Flags: needinfo?(jgilbert)
Blocks: webgl2-features
Assignee: nobody → jgilbert
Good news for me:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fbf0851afc41

Bad news for jrmuizel forthcoming.
Attached file Bug 1250710 - Add webidl. - (deleted) — 
Review commit: https://reviewboard.mozilla.org/r/59860/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/59860/
Attachment #8763729 - Flags: review?(ehsan)
Attachment #8763730 - Flags: review?(jmuizelaar)
Attachment #8763731 - Flags: review?(jmuizelaar)
Attachment #8763732 - Flags: review?(jmuizelaar)
Attachment #8763733 - Flags: review?(jmuizelaar)
Attachment #8763734 - Flags: review?(jmuizelaar)
Attachment #8763735 - Flags: review?(jmuizelaar)
Attachment #8763736 - Flags: review?(jmuizelaar)
Attachment #8763737 - Flags: review?(jmuizelaar)
Attachment #8763738 - Flags: review?(jmuizelaar)
Attachment #8763739 - Flags: review?(jmuizelaar)
Attachment #8763740 - Flags: review?(jmuizelaar)
Attachment #8763741 - Flags: review?(jmuizelaar)
Attachment #8763742 - Flags: review?(jmuizelaar)
Attachment #8763743 - Flags: review?(jmuizelaar)
Attachment #8763744 - Flags: review?(jmuizelaar)
Attachment #8763745 - Flags: review?(jmuizelaar)
Attachment #8763746 - Flags: review?(jmuizelaar)
Attachment #8763747 - Flags: review?(jmuizelaar)
Attachment #8763748 - Flags: review?(jmuizelaar)
This bug has low priority.
Severity: normal → minor
Blocks: webgl2-blockers
No longer blocks: webgl2-blockers
Depends on: 1284346
Comment on attachment 8763729 [details]
Bug 1250710 - Add webidl. -

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59860/diff/1-2/
Attachment #8763732 - Attachment description: Bug 1250710 - Refactor out common ReadPixels validation. - → Bug 1250710 - Add PACK PBO support. -
Attachment #8763733 - Attachment description: Bug 1250710 - Implement ReadPixels for PBOs. - → Bug 1250710 - Remove unused functions. -
Attachment #8763741 - Attachment description: Bug 1250710 - Ensure bound PBO iff needed. - → Bug 1250710 - Mark specific InvalidEnum case. -
Attachment #8763729 - Flags: review?(ehsan) → review?(bugs)
Comment on attachment 8763730 [details]
Bug 1250710 - Add stub. -

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59862/diff/1-2/
Comment on attachment 8763731 [details]
Bug 1250710 - Clean up formatting. -

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59864/diff/1-2/
Comment on attachment 8763734 [details]
Bug 1250710 - Add a test. -

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59870/diff/1-2/
Comment on attachment 8763732 [details]
Bug 1250710 - Add PACK PBO support. -

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59866/diff/1-2/
Comment on attachment 8763735 [details]
Bug 1250710 - Unlock PIXEL_PACK_BUFFER. -

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59872/diff/1-2/
Comment on attachment 8763736 [details]
Bug 1250710 - Update scoped pack/unpack helpers. -

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59874/diff/1-2/
Comment on attachment 8763737 [details]
Bug 1250710 - Remove redundant IsCurrent checks. -

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59876/diff/1-2/
Comment on attachment 8763738 [details]
Bug 1250710 - Remove packAlign save/restore from ScopedGLDrawState. -

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59878/diff/1-2/
Comment on attachment 8763739 [details]
Bug 1250710 - Update usages of ScopedPackAlignment. -

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59880/diff/1-2/
Comment on attachment 8763740 [details]
Bug 1250710 - Remove ScopedUnpackState due to unused. -

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59882/diff/1-2/
Comment on attachment 8763745 [details]
Bug 1250710 - User of ReadPixel should clear PBO to 0. -

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59892/diff/1-2/
Comment on attachment 8763748 [details]
Bug 1250710 - Save and restore state, since this is used by WebGL. -

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59898/diff/1-2/
Comment on attachment 8763744 [details]
Bug 1250710 - Workaround nvidia when stride is longer than the last row with PBOs. -

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59890/diff/1-2/
Comment on attachment 8763733 [details]
Bug 1250710 - Remove unused functions. -

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59868/diff/1-2/
Comment on attachment 8763741 [details]
Bug 1250710 - Mark specific InvalidEnum case. -

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59884/diff/1-2/
Attachment #8763742 - Attachment is obsolete: true
Attachment #8763742 - Flags: review?(jmuizelaar)
Attachment #8763743 - Attachment is obsolete: true
Attachment #8763743 - Flags: review?(jmuizelaar)
Attachment #8763746 - Attachment is obsolete: true
Attachment #8763746 - Flags: review?(jmuizelaar)
Attachment #8763747 - Attachment is obsolete: true
Attachment #8763747 - Flags: review?(jmuizelaar)
Attachment #8763730 - Flags: review?(jmuizelaar) → review?(ethlin)
Attachment #8763731 - Flags: review?(jmuizelaar) → review?(ethlin)
Attachment #8763734 - Flags: review?(jmuizelaar) → review?(ethlin)
Attachment #8763735 - Flags: review?(jmuizelaar) → review?(ethlin)
Attachment #8763737 - Flags: review?(jmuizelaar) → review?(ethlin)
Attachment #8763740 - Flags: review?(jmuizelaar) → review?(ethlin)
Attachment #8763745 - Flags: review?(jmuizelaar) → review?(ethlin)
Attachment #8763748 - Flags: review?(jmuizelaar) → review?(ethlin)
Attachment #8763733 - Flags: review?(jmuizelaar) → review?(ethlin)
Attachment #8763741 - Flags: review?(jmuizelaar) → review?(ethlin)
Attachment #8767848 - Flags: review?(jmuizelaar) → review?(ethlin)
Comment on attachment 8763729 [details]
Bug 1250710 - Add webidl. -

r=khuey via IRC
Attachment #8763729 - Flags: review?(bugs) → review+
Blocks: webgl2-blockers
Comment on attachment 8763732 [details]
Bug 1250710 - Add PACK PBO support. -

https://reviewboard.mozilla.org/r/59866/#review60756

::: dom/canvas/WebGLContextGL.cpp:1406
(Diff revision 2)
>      default:
>          return false;
>      }
>  }
>  
> +static bool

This should return Maybe<js::Scalar::Type>

::: dom/canvas/WebGLFormats.cpp:339
(Diff revision 2)
>  }
>  
>  //////////////////////////////////////////////////////////////////////////////////////////
>  
> -uint8_t
> -BytesPerPixel(const PackingInfo& packing)
> +bool
> +GetBytesPerPixel(const PackingInfo& packing, uint8_t* const out_bytes)

This should return Maybe<uint8_t>
Attachment #8763732 - Flags: review?(jmuizelaar) → review+
Comment on attachment 8763736 [details]
Bug 1250710 - Update scoped pack/unpack helpers. -

https://reviewboard.mozilla.org/r/59874/#review60764
Attachment #8763736 - Flags: review?(jmuizelaar) → review+
Comment on attachment 8763739 [details]
Bug 1250710 - Update usages of ScopedPackAlignment. -

https://reviewboard.mozilla.org/r/59880/#review60766
Attachment #8763739 - Flags: review?(jmuizelaar) → review+
Attachment #8763744 - Flags: review?(jmuizelaar) → review+
Comment on attachment 8763744 [details]
Bug 1250710 - Workaround nvidia when stride is longer than the last row with PBOs. -

https://reviewboard.mozilla.org/r/59890/#review60768

::: dom/canvas/WebGLContextGL.cpp:1309
(Diff revision 2)
>          }
>  
>          return true;
>      }
>  
> +    // On at least Win+NV, we'll get PBO errors if we don't handle this:

A summary like:
// On at least Win+NV, we'll get PBO errors if we don't have at least rowStride * height bytes available to read into.

::: dom/canvas/WebGLContextGL.cpp:1321
(Diff revision 2)
> +    if (!useParanoidHandling) {
> -    gl->fReadPixels(x, y, width, height, format, destType, dest);
> +        gl->fReadPixels(x, y, width, height, format, destType, dest);
> -    return true;
> +        return true;
> -}
> +    }
>  
> +    const auto bodyHeight = height - 1;

This could use comments like:
// Read everything but the last row

::: dom/canvas/WebGLContextGL.cpp:1329
(Diff revision 2)
> +    }
> +
> +    gl->fPixelStorei(LOCAL_GL_PACK_ALIGNMENT, 1);
> +    gl->fPixelStorei(LOCAL_GL_PACK_ROW_LENGTH, 0);
> +    gl->fPixelStorei(LOCAL_GL_PACK_SKIP_ROWS, 0);
> +

// read the last row
Comment on attachment 8763738 [details]
Bug 1250710 - Remove packAlign save/restore from ScopedGLDrawState. -

https://reviewboard.mozilla.org/r/59878/#review60776

::: gfx/gl/ScopedGLHelpers.cpp:443
(Diff revision 2)
>      mGL->GetUIntegerv(LOCAL_GL_CURRENT_PROGRAM, &boundProgram);
>      mGL->GetUIntegerv(LOCAL_GL_ARRAY_BUFFER_BINDING, &boundBuffer);
>      mGL->GetUIntegerv(LOCAL_GL_MAX_VERTEX_ATTRIBS, &maxAttrib);
>      attrib_enabled = MakeUnique<GLint[]>(maxAttrib);
>  
> -    for (unsigned int i = 0; i < maxAttrib; i++) {
> +    for (GLuint i = 0; i < maxAttrib; i++) {

This change doesn't belong here.
Attachment #8763738 - Flags: review?(jmuizelaar) → review+
Comment on attachment 8763731 [details]
Bug 1250710 - Clean up formatting. -

https://reviewboard.mozilla.org/r/59864/#review60900
Attachment #8763731 - Flags: review?(ethlin) → review+
Attachment #8763735 - Flags: review?(ethlin) → review+
Comment on attachment 8763737 [details]
Bug 1250710 - Remove redundant IsCurrent checks. -

https://reviewboard.mozilla.org/r/59876/#review60904
Attachment #8763737 - Flags: review?(ethlin) → review+
Comment on attachment 8763740 [details]
Bug 1250710 - Remove ScopedUnpackState due to unused. -

https://reviewboard.mozilla.org/r/59882/#review60920

The m-c doesn't have ScopedUnpackState. So you just remove the struct you added in another patch.
Attachment #8763740 - Flags: review?(ethlin) → review+
Comment on attachment 8763745 [details]
Bug 1250710 - User of ReadPixel should clear PBO to 0. -

https://reviewboard.mozilla.org/r/59892/#review60922
Attachment #8763745 - Flags: review?(ethlin) → review+
(In reply to Ethan Lin[:ethlin] from comment #55)
> Comment on attachment 8763740 [details]
> Bug 1250710 - Remove ScopedUnpackState due to unused. -
> 
> https://reviewboard.mozilla.org/r/59882/#review60920
> 
> The m-c doesn't have ScopedUnpackState. So you just remove the struct you
> added in another patch.

Oh, you just renamed it.
Attachment #8763748 - Flags: review?(ethlin) → review+
Comment on attachment 8763748 [details]
Bug 1250710 - Save and restore state, since this is used by WebGL. -

https://reviewboard.mozilla.org/r/59898/#review60924

::: gfx/gl/GLBlitHelper.cpp:908
(Diff revision 2)
>          return BlitGrallocImage(static_cast<layers::GrallocImage*>(srcImage));
>  #endif
>  
> -    case ConvertPlanarYCbCr:
> +    case ConvertPlanarYCbCr: {
> +            const auto saved = mGL->GetIntAs<GLint>(LOCAL_GL_UNPACK_ALIGNMENT);
> -        mGL->fPixelStorei(LOCAL_GL_UNPACK_ALIGNMENT, 1);
> +            mGL->fPixelStorei(LOCAL_GL_UNPACK_ALIGNMENT, 1);

Why don't you use ScopedUnpackState here?
Comment on attachment 8763733 [details]
Bug 1250710 - Remove unused functions. -

https://reviewboard.mozilla.org/r/59868/#review60926
Attachment #8763733 - Flags: review?(ethlin) → review+
Comment on attachment 8763741 [details]
Bug 1250710 - Mark specific InvalidEnum case. -

https://reviewboard.mozilla.org/r/59884/#review60930
Attachment #8763741 - Flags: review?(ethlin) → review+
Attachment #8767848 - Flags: review?(ethlin) → review+
Comment on attachment 8767848 [details]
Bug 1250710 - ANGLE ES2 still requires HALF_FLOAT for ReadPixels, not HALF_FLOAT_OES. -

https://reviewboard.mozilla.org/r/62234/#review60932
https://reviewboard.mozilla.org/r/59870/#review60934

Shouldn't you add the test to mochitest.ini?

::: dom/canvas/WebGL2ContextBuffers.cpp:236
(Diff revision 2)
>                                      LOCAL_GL_MAP_READ_BIT);
>      // Warning: Possibly shared memory.  See bug 1225033.
>      memcpy(data.DataAllowShared(), ptr, data.LengthAllowShared());
>      gl->fUnmapBuffer(target);
>  
> +

It should be an unnecessary line.
Attachment #8763734 - Flags: review?(ethlin) → review+
Comment on attachment 8763734 [details]
Bug 1250710 - Add a test. -

https://reviewboard.mozilla.org/r/59870/#review60944

r+ if the issues are addressed.

::: dom/canvas/test/webgl-mochitest/test_pixel_pack_buffer.html:1
(Diff revision 2)
> +<!DOCTYPE HTML>

Please also change the mochitest.ini.

::: dom/canvas/test/webgl-mochitest/test_pixel_pack_buffer.html:147
(Diff revision 2)
> +
> +  TestIsUNormColor(RED, new Uint8Array([255, 0, 0, 255]), 0);
> +
> +  ////////
> +
> +  gl.clearColor(RED[0], RED[1], RED[2], RED[3]);

duplicate clearColor
Pushed by jgilbert@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2dee8c10e507
Add webidl. - r=khuey
https://hg.mozilla.org/integration/mozilla-inbound/rev/626fa1c3e88c
Add stub. - r=ethlin
https://hg.mozilla.org/integration/mozilla-inbound/rev/cb9ec5bb8bcc
Clean up formatting. - r=ethlin
https://hg.mozilla.org/integration/mozilla-inbound/rev/c2ee198a1c71
Add a test. - r=ethlin
https://hg.mozilla.org/integration/mozilla-inbound/rev/385d885ca02f
Add PACK PBO support. - r=jrmuizel
https://hg.mozilla.org/integration/mozilla-inbound/rev/057ec3ca3680
Unlock PIXEL_PACK_BUFFER. - r=ethlin
https://hg.mozilla.org/integration/mozilla-inbound/rev/f99eeb836982
Update scoped pack/unpack helpers. - r=jrmuizel
https://hg.mozilla.org/integration/mozilla-inbound/rev/4423ce67bfb7
Remove redundant IsCurrent checks. - r=ethlin
https://hg.mozilla.org/integration/mozilla-inbound/rev/627980e477d3
Remove packAlign save/restore from ScopedGLDrawState. - r=jrmuizel
https://hg.mozilla.org/integration/mozilla-inbound/rev/ef99639a073c
Update usages of ScopedPackAlignment. - r=jrmuizel
https://hg.mozilla.org/integration/mozilla-inbound/rev/c8b9fe689ada
Remove ScopedUnpackState due to unused. - r=ethlin
https://hg.mozilla.org/integration/mozilla-inbound/rev/2fee18feb623
User of ReadPixel should clear PBO to 0. - r=ethlin
https://hg.mozilla.org/integration/mozilla-inbound/rev/c70eb1338bf4
Save and restore state, since this is used by WebGL. - r=ethlin
https://hg.mozilla.org/integration/mozilla-inbound/rev/40e9d24fd8ea
Workaround nvidia when stride is longer than the last row with PBOs. - r=jrmuizel
https://hg.mozilla.org/integration/mozilla-inbound/rev/0a20a55213d2
Remove unused functions. - r=ethlin
https://hg.mozilla.org/integration/mozilla-inbound/rev/43608cd94c6f
Mark specific InvalidEnum case. - r=ethlin
https://hg.mozilla.org/integration/mozilla-inbound/rev/e8b72c8f59f9
ANGLE ES2 still requires HALF_FLOAT for ReadPixels, not HALF_FLOAT_OES. - r=ethlin
https://hg.mozilla.org/integration/mozilla-inbound/rev/b658098634b8
Call SimpleTest.finish() when skipping due to lack of WebGL2.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: