Bug 177119

Summary: Creating a second AVPlayerItemVideoOutput causes flakey failures
Product: WebKit Reporter: Dean Jackson <dino>
Component: MediaAssignee: Jer Noble <jer.noble>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, buildbot, commit-queue, eric.carlson, ews-watchlist, jeremyj-wk, jer.noble, jlewis3, rniwa, ryanhaddad
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
buildbot: commit-queue-
Archive of layout-test-results from ews100 for mac-elcapitan
none
Archive of layout-test-results from ews104 for mac-elcapitan-wk2
none
Archive of layout-test-results from ews112 for mac-elcapitan
none
Archive of layout-test-results from ews125 for ios-simulator-wk2
none
Patch
none
Archive of layout-test-results from ews103 for mac-elcapitan
none
Archive of layout-test-results from ews107 for mac-elcapitan-wk2
none
Archive of layout-test-results from ews115 for mac-elcapitan
none
Patch
none
Archive of layout-test-results from ews103 for mac-elcapitan
none
Archive of layout-test-results from ews105 for mac-elcapitan-wk2
none
Archive of layout-test-results from ews116 for mac-elcapitan
none
Archive of layout-test-results from ews126 for ios-simulator-wk2
none
Patch
none
Archive of layout-test-results from ews125 for ios-simulator-wk2
none
Patch
none
Patch for landing none

Description Dean Jackson 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?
Comment 1 Dean Jackson 2017-09-18 19:39:21 PDT
<rdar://problem/34507977>
Comment 2 Jer Noble 2017-09-19 10:18:43 PDT
Created attachment 321207 [details]
Patch
Comment 3 Build Bot 2017-09-19 11:27:32 PDT Comment hidden (obsolete)
Comment 4 Build Bot 2017-09-19 11:27:34 PDT Comment hidden (obsolete)
Comment 5 Build Bot 2017-09-19 11:30:07 PDT Comment hidden (obsolete)
Comment 6 Build Bot 2017-09-19 11:30:08 PDT Comment hidden (obsolete)
Comment 7 Build Bot 2017-09-19 11:48:46 PDT Comment hidden (obsolete)
Comment 8 Build Bot 2017-09-19 11:48:48 PDT Comment hidden (obsolete)
Comment 9 Build Bot 2017-09-19 11:54:26 PDT Comment hidden (obsolete)
Comment 10 Build Bot 2017-09-19 11:54:27 PDT Comment hidden (obsolete)
Comment 11 Jer Noble 2017-11-17 14:00:35 PST Comment hidden (obsolete)
Comment 12 EWS Watchlist 2017-11-17 14:50:59 PST Comment hidden (obsolete)
Comment 13 EWS Watchlist 2017-11-17 14:51:00 PST Comment hidden (obsolete)
Comment 14 EWS Watchlist 2017-11-17 15:07:40 PST Comment hidden (obsolete)
Comment 15 EWS Watchlist 2017-11-17 15:07:42 PST Comment hidden (obsolete)
Comment 16 EWS Watchlist 2017-11-17 16:03:55 PST Comment hidden (obsolete)
Comment 17 EWS Watchlist 2017-11-17 16:03:56 PST Comment hidden (obsolete)
Comment 18 Jer Noble 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).
Comment 19 Jer Noble 2017-12-01 23:13:41 PST
Created attachment 328217 [details]
Patch
Comment 20 EWS Watchlist 2017-12-01 23:59:42 PST Comment hidden (obsolete)
Comment 21 EWS Watchlist 2017-12-01 23:59:43 PST Comment hidden (obsolete)
Comment 22 EWS Watchlist 2017-12-02 00:27:06 PST Comment hidden (obsolete)
Comment 23 EWS Watchlist 2017-12-02 00:27:08 PST Comment hidden (obsolete)
Comment 24 EWS Watchlist 2017-12-02 00:37:44 PST Comment hidden (obsolete)
Comment 25 EWS Watchlist 2017-12-02 00:37:45 PST Comment hidden (obsolete)
Comment 26 EWS Watchlist 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
Comment 27 EWS Watchlist 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
Comment 28 Jer Noble 2017-12-02 08:13:07 PST
Created attachment 328231 [details]
Patch
Comment 29 EWS Watchlist 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
Comment 30 EWS Watchlist 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
Comment 31 Jer Noble 2017-12-02 16:29:55 PST
And now it looks like iOS is getting the color channels reversed
Comment 32 Jer Noble 2017-12-05 08:33:12 PST
Created attachment 328452 [details]
Patch
Comment 33 Dean Jackson 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 :)
Comment 34 Jer Noble 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!
Comment 35 Jer Noble 2017-12-07 11:11:56 PST
Created attachment 328709 [details]
Patch for landing
Comment 36 WebKit Commit Bot 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.
Comment 37 WebKit Commit Bot 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>
Comment 38 WebKit Commit Bot 2017-12-07 11:56:33 PST
All reviewed patches have been landed.  Closing bug.