RESOLVED FIXED 177119
Creating a second AVPlayerItemVideoOutput causes flakey failures
https://bugs.webkit.org/show_bug.cgi?id=177119
Summary Creating a second AVPlayerItemVideoOutput causes flakey failures
Dean Jackson
Reported 2017-09-18 19:39:05 PDT
Since landing the flipY patch (176491), the WebGL conformance test that exercises video -> texture is flakey. This is strange because the test never actually exercises the GPU path: hasNewPixelBufferForItemTime always returns false. This happens in Safari too, as if it takes a little while before there is output to actually read. LayoutTests/webgl/1.0.2/conformance/textures/tex-image-and-sub-image-2d-with-video.html As far as I can tell, the first time through we create the AVPlayerItemVideoOutput, but do not have hasNewPixelBufferForItemTime. The second time through we have the AVPlayerItemVideoOutput, but still don't have hasNewPixelBufferForItemTime. The third time through we get true from hasNewPixelBufferForItemTime, and the fast path produces the texture. We never hit this before because the previous code returned immediately if flipY was set, so it instantly fell back to the software path and never tried to create the AVPlayerItemVideoOutput. The failure is that the software path fails to produce a texture, simply uploading black. It doesn't produce an error, so we think it works. Can you think of a reason why creating the AVPlayerItemVideoOutput and adding it to the AVPlayerItem would cause another output to stop working, but only sometimes? Note that even returning early from MediaPlayerPrivateAVFoundationObjC::copyVideoTextureToPlatformTexture, before we ask the output if there is a frame ready, causes the failure. It's just whether or not we created the AVPlayerItemVideoOutput. The other thing to understand is why the AVPlayerItemVideoOutput doesn't have a pixel buffer available right away. I guess that's because it's waiting for a frame to decode?
Attachments
Patch (10.21 KB, patch)
2017-09-19 10:18 PDT, Jer Noble
buildbot: commit-queue-
Archive of layout-test-results from ews100 for mac-elcapitan (1.37 MB, application/zip)
2017-09-19 11:27 PDT, Build Bot
no flags
Archive of layout-test-results from ews104 for mac-elcapitan-wk2 (1.53 MB, application/zip)
2017-09-19 11:30 PDT, Build Bot
no flags
Archive of layout-test-results from ews112 for mac-elcapitan (2.21 MB, application/zip)
2017-09-19 11:48 PDT, Build Bot
no flags
Archive of layout-test-results from ews125 for ios-simulator-wk2 (1.43 MB, application/zip)
2017-09-19 11:54 PDT, Build Bot
no flags
Patch (31.45 KB, patch)
2017-11-17 14:00 PST, Jer Noble
no flags
Archive of layout-test-results from ews103 for mac-elcapitan (2.25 MB, application/zip)
2017-11-17 14:51 PST, EWS Watchlist
no flags
Archive of layout-test-results from ews107 for mac-elcapitan-wk2 (3.05 MB, application/zip)
2017-11-17 15:07 PST, EWS Watchlist
no flags
Archive of layout-test-results from ews115 for mac-elcapitan (3.34 MB, application/zip)
2017-11-17 16:03 PST, EWS Watchlist
no flags
Patch (33.09 KB, patch)
2017-12-01 23:13 PST, Jer Noble
no flags
Archive of layout-test-results from ews103 for mac-elcapitan (2.42 MB, application/zip)
2017-12-01 23:59 PST, EWS Watchlist
no flags
Archive of layout-test-results from ews105 for mac-elcapitan-wk2 (2.64 MB, application/zip)
2017-12-02 00:27 PST, EWS Watchlist
no flags
Archive of layout-test-results from ews116 for mac-elcapitan (2.87 MB, application/zip)
2017-12-02 00:37 PST, EWS Watchlist
no flags
Archive of layout-test-results from ews126 for ios-simulator-wk2 (2.11 MB, application/zip)
2017-12-02 00:38 PST, EWS Watchlist
no flags
Patch (353.67 KB, patch)
2017-12-02 08:13 PST, Jer Noble
no flags
Archive of layout-test-results from ews125 for ios-simulator-wk2 (2.14 MB, application/zip)
2017-12-02 09:44 PST, EWS Watchlist
no flags
Patch (360.78 KB, patch)
2017-12-05 08:33 PST, Jer Noble
no flags
Patch for landing (360.88 KB, patch)
2017-12-07 11:11 PST, Jer Noble
no flags
Dean Jackson
Comment 1 2017-09-18 19:39:21 PDT
Jer Noble
Comment 2 2017-09-19 10:18:43 PDT
Build Bot
Comment 3 2017-09-19 11:27:32 PDT Comment hidden (obsolete)
Build Bot
Comment 4 2017-09-19 11:27:34 PDT Comment hidden (obsolete)
Build Bot
Comment 5 2017-09-19 11:30:07 PDT Comment hidden (obsolete)
Build Bot
Comment 6 2017-09-19 11:30:08 PDT Comment hidden (obsolete)
Build Bot
Comment 7 2017-09-19 11:48:46 PDT Comment hidden (obsolete)
Build Bot
Comment 8 2017-09-19 11:48:48 PDT Comment hidden (obsolete)
Build Bot
Comment 9 2017-09-19 11:54:26 PDT Comment hidden (obsolete)
Build Bot
Comment 10 2017-09-19 11:54:27 PDT Comment hidden (obsolete)
Jer Noble
Comment 11 2017-11-17 14:00:35 PST Comment hidden (obsolete)
EWS Watchlist
Comment 12 2017-11-17 14:50:59 PST Comment hidden (obsolete)
EWS Watchlist
Comment 13 2017-11-17 14:51:00 PST Comment hidden (obsolete)
EWS Watchlist
Comment 14 2017-11-17 15:07:40 PST Comment hidden (obsolete)
EWS Watchlist
Comment 15 2017-11-17 15:07:42 PST Comment hidden (obsolete)
EWS Watchlist
Comment 16 2017-11-17 16:03:55 PST Comment hidden (obsolete)
EWS Watchlist
Comment 17 2017-11-17 16:03:56 PST Comment hidden (obsolete)
Jer Noble
Comment 18 2017-11-17 16:44:04 PST
(In reply to Build Bot from comment #14) > Comment on attachment 327233 [details] > Patch > > Attachment 327233 [details] did not pass mac-wk2-ews (mac-wk2): > Output: http://webkit-queues.webkit.org/results/5281052 > > New failing tests: > fast/canvas/webgl/texImage2D-video-flipY-true.html > fast/canvas/webgl/texImage2D-mse-flipY-true.html > fast/canvas/webgl/texImage2D-mse-flipY-false.html Looks like these tests are failing because the blue is not quite blue enough and the red is not quite red enough (i.e., the red channel in the blue test is 20/255 when it's testing for <20/255).
Jer Noble
Comment 19 2017-12-01 23:13:41 PST
EWS Watchlist
Comment 20 2017-12-01 23:59:42 PST Comment hidden (obsolete)
EWS Watchlist
Comment 21 2017-12-01 23:59:43 PST Comment hidden (obsolete)
EWS Watchlist
Comment 22 2017-12-02 00:27:06 PST Comment hidden (obsolete)
EWS Watchlist
Comment 23 2017-12-02 00:27:08 PST Comment hidden (obsolete)
EWS Watchlist
Comment 24 2017-12-02 00:37:44 PST Comment hidden (obsolete)
EWS Watchlist
Comment 25 2017-12-02 00:37:45 PST Comment hidden (obsolete)
EWS Watchlist
Comment 26 2017-12-02 00:38:17 PST
Comment on attachment 328217 [details] Patch Attachment 328217 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/5464516 New failing tests: fast/canvas/webgl/tex-image-and-sub-image-2d-with-video.html
EWS Watchlist
Comment 27 2017-12-02 00:38:18 PST
Created attachment 328224 [details] Archive of layout-test-results from ews126 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews126 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.6
Jer Noble
Comment 28 2017-12-02 08:13:07 PST
EWS Watchlist
Comment 29 2017-12-02 09:44:23 PST
Comment on attachment 328231 [details] Patch Attachment 328231 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/5467367 New failing tests: fast/canvas/webgl/tex-image-and-sub-image-2d-with-video.html
EWS Watchlist
Comment 30 2017-12-02 09:44:25 PST
Created attachment 328236 [details] Archive of layout-test-results from ews125 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews125 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.6
Jer Noble
Comment 31 2017-12-02 16:29:55 PST
And now it looks like iOS is getting the color channels reversed
Jer Noble
Comment 32 2017-12-05 08:33:12 PST
Dean Jackson
Comment 33 2017-12-06 16:29:08 PST
Comment on attachment 328452 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=328452&action=review > Source/WebCore/ChangeLog:39 > + uploading BGRA pixel formatted data to a texture; work aronud this bug for now. Typo: around > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:2333 > - if (!pixelBuffer) > + if (!updateLastPixelBuffer() && (m_lastImage || !m_lastPixelBuffer)) I don't quite understand the logic change here. The !pixelBuffer and !updateLastPixelBuffer() are effectively the same thing... but why did you add && (m_lastImage || !m_lastPixelBuffer)? > Source/WebCore/platform/graphics/cv/VideoTextureCopierCV.cpp:329 > + fragmentShaderSource.appendLiteral("uniform int u_swapColors;\n"); naming: flipColorChannels? swapColorChannels? > Source/WebCore/platform/graphics/cv/VideoTextureCopierCV.cpp:452 > - " yuv.gb = TEXTUREFUNC(u_uvTexture, v_uvTextureCoordinate).rg - vec2(0.5, 0.5);\n" > - " gl_FragColor = vec4(yuv * u_colorMatrix, 1);\n" > + " yuv.gb = TEXTUREFUNC(u_uvTexture, v_uvTextureCoordinate).rg;\n" Why aren't you subtracting 0.5, 0.5 any more? > Source/WebCore/platform/graphics/cv/VideoTextureCopierCV.cpp:-723 > -VideoTextureCopierCV::GC3DStateSaver::GC3DStateSaver(GraphicsContext3D& context) > - : m_context(context) Booohooooo.... this had so much promise :)
Jer Noble
Comment 34 2017-12-06 16:36:53 PST
Comment on attachment 328452 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=328452&action=review >> Source/WebCore/ChangeLog:39 >> + uploading BGRA pixel formatted data to a texture; work aronud this bug for now. > > Typo: around Will fix. >> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:2333 >> + if (!updateLastPixelBuffer() && (m_lastImage || !m_lastPixelBuffer)) > > I don't quite understand the logic change here. The !pixelBuffer and !updateLastPixelBuffer() are effectively the same thing... but why did you add && (m_lastImage || !m_lastPixelBuffer)? The logic is: "If updateLastPixelBuffer() didn't end up with anything new (and returned false), and we either already converted the last valid m_lastPixelBuffer to an m_lastImage, or there's no m_lastPixelBuffer in the first place, then bail out early." If we didn't get a new m_lastPixelBuffer, and we also don't have a m_lastImage, then we should _not_ do an early return and do the YUV->RGB conversion in the conformer below. The key (I think) to understanding why we do this check are the lines above in updateLastPixelBuffer(): > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:2310 > + m_lastPixelBuffer = adoptCF([m_videoOutput.get() copyPixelBufferForItemTime:currentTime itemTimeForDisplay:nil]); > + m_lastImage = nullptr; Whenever we update m_lastPixelBuffer with a new value, we clear out m_lastImage. So a call to paint into a WebGL canvas context will result in a non-null m_lastPixelBuffer, and a subsequent call to paint into a 2d canvas context will conform that existing m_lastPixelBuffer into a m_lastImage. >> Source/WebCore/platform/graphics/cv/VideoTextureCopierCV.cpp:452 >> + " yuv.gb = TEXTUREFUNC(u_uvTexture, v_uvTextureCoordinate).rg;\n" > > Why aren't you subtracting 0.5, 0.5 any more? The subtraction is now part of the u_colorMatrix in the "w" column. >> Source/WebCore/platform/graphics/cv/VideoTextureCopierCV.cpp:-723 >> - : m_context(context) > > Booohooooo.... this had so much promise :) It did!
Jer Noble
Comment 35 2017-12-07 11:11:56 PST
Created attachment 328709 [details] Patch for landing
WebKit Commit Bot
Comment 36 2017-12-07 11:55:50 PST
The commit-queue encountered the following flaky tests while processing attachment 328709 [details]: http/tests/xmlhttprequest/redirect-cross-origin-tripmine.html bug 179840 (authors: ap@webkit.org and rniwa@webkit.org) The commit-queue is continuing to process your patch.
WebKit Commit Bot
Comment 37 2017-12-07 11:56:31 PST
Comment on attachment 328709 [details] Patch for landing Clearing flags on attachment: 328709 Committed r225638: <https://trac.webkit.org/changeset/225638>
WebKit Commit Bot
Comment 38 2017-12-07 11:56:33 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.