Summary: | iOS 12.2 Drawing portrait video to canvas is sideways | ||
---|---|---|---|
Product: | WebKit | Reporter: | Doug Parker <douglasparker> |
Component: | Canvas | Assignee: | Jer Noble <jer.noble> |
Status: | RESOLVED FIXED | ||
Severity: | Normal | CC: | commit-queue, dino, eric.carlson, ews-watchlist, jer.noble, jonlee, repstein, rniwa, tsavell, webkit-bug-importer |
Priority: | P2 | Keywords: | InRadar |
Version: | Safari 12 | ||
Hardware: | iPhone / iPad | ||
OS: | iOS 12 | ||
Attachments: |
Description
Doug Parker
2019-04-10 09:44:10 PDT
Created attachment 367131 [details]
Bug reproducing in production Bulletin
I should probably also add that desktop and Android Chrome both render the canvas in the correct orientation. Safari 12.1 also did not seem to reproduce the problem, so this appears to be a WebKit/Safari 12.2 specific problem. The bug also reproduces on Desktop Safari with a portrait video taken on iOS. Looks like the video in question has a video track preferredTransform of: (a = 0, b = 1, c = -1, d = 0, tx = 1080, ty = 0), so rotated. I suspect that prior to the release of iOS 12.2, videos taken in portrait mode had an identity transform, and that if you played a portrait video captured in iOS 12.1 in the jsfiddle using iOS 12.2, everything would be correctly oriented. We handle transformed tracks for MSE content by explicitly rotating the pixel buffers generated by a decompression session. We may need to do the same for AVAsset-backed video elements. Created attachment 372278 [details]
Patch
Comment on attachment 372278 [details] Patch Attachment 372278 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/12501362 New failing tests: media/media-source/only-bcp47-language-tags-accepted-as-valid.html Created attachment 372283 [details]
Archive of layout-test-results from ews102 for mac-highsierra
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102 Port: mac-highsierra Platform: Mac OS X 10.13.6
Created attachment 372291 [details]
Patch
Attachment 372291 [details] did not pass style-queue:
ERROR: Source/WebCore/platform/graphics/cv/ImageRotationSessionVT.mm:32: *SoftLink.h header should be included after all other headers. [build/include_order] [4]
Total errors found: 1 in 15 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 372291 [details] Patch Attachment 372291 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/12502014 New failing tests: media/media-source/only-bcp47-language-tags-accepted-as-valid.html Created attachment 372296 [details]
Archive of layout-test-results from ews103 for mac-highsierra
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103 Port: mac-highsierra Platform: Mac OS X 10.13.6
Comment on attachment 372291 [details] Patch Attachment 372291 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/12502252 New failing tests: media/media-source/only-bcp47-language-tags-accepted-as-valid.html Created attachment 372301 [details]
Archive of layout-test-results from ews104 for mac-highsierra-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104 Port: mac-highsierra-wk2 Platform: Mac OS X 10.13.6
Comment on attachment 372291 [details] Patch Attachment 372291 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/12502235 New failing tests: media/media-source/only-bcp47-language-tags-accepted-as-valid.html Created attachment 372305 [details]
Archive of layout-test-results from ews113 for mac-highsierra
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113 Port: mac-highsierra Platform: Mac OS X 10.13.6
From the test results: -CONSOLE MESSAGE: line 221: The language 'a' is not a valid BCP 47 language tag. +CONSOLE MESSAGE: line 222: The language 'a' is not a valid BCP 47 language tag. These are all just line number changes, triggered by modifying video-test.js. Will rebase line. Created attachment 372372 [details]
Patch
Comment on attachment 372372 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=372372&action=review > Source/WebCore/platform/graphics/avfoundation/objc/ImageDecoderAVFObjC.mm:428 > + if (m_imageRotationSession) Might be worth adding ASSERT(m_imageRotationSession) here to catch this in debug builds. > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:1863 > + if (!finalTransform.isIdentity()) > + m_imageRotationSession = std::make_unique<ImageRotationSessionVT>(WTFMove(finalTransform), expandedIntSize(m_cachedPresentationSize), kCVPixelFormatType_32BGRA, ImageRotationSessionVT::IsCGImageCompatible::Yes); > + else > + m_imageRotationSession = nullptr; Tracks change so fequently with HLS that it might be worth doing this only when m_cachedPresentationSize changes. Also, shouldn't we also do this when MediaPlayerPrivateAVFoundationObjC::sizeChanged is called? > Source/WebCore/platform/graphics/cv/ImageRotationSessionVT.mm:92 > + CVPixelBufferPoolCreate(kCFAllocatorDefault, nullptr, (__bridge CFDictionaryRef)pixelAttributes, &rawPool); Even if it is safe to assume it is OK to ignore this failing, we should log an error when it does. (In reply to Eric Carlson from comment #19) > Comment on attachment 372372 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=372372&action=review > > > Source/WebCore/platform/graphics/avfoundation/objc/ImageDecoderAVFObjC.mm:428 > > + if (m_imageRotationSession) > > Might be worth adding ASSERT(m_imageRotationSession) here to catch this in > debug builds. No, this is fine: if the movie and track transforms are both identity, then we won't create a rotation session. > > Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:1863 > > + if (!finalTransform.isIdentity()) > > + m_imageRotationSession = std::make_unique<ImageRotationSessionVT>(WTFMove(finalTransform), expandedIntSize(m_cachedPresentationSize), kCVPixelFormatType_32BGRA, ImageRotationSessionVT::IsCGImageCompatible::Yes); > > + else > > + m_imageRotationSession = nullptr; > > Tracks change so fequently with HLS that it might be worth doing this only > when m_cachedPresentationSize changes. Sure. > Also, shouldn't we also do this when > MediaPlayerPrivateAVFoundationObjC::sizeChanged is called? Oooh, will we ever get a size change without an associated track change? Maybe we will want to do this there then. > > Source/WebCore/platform/graphics/cv/ImageRotationSessionVT.mm:92 > > + CVPixelBufferPoolCreate(kCFAllocatorDefault, nullptr, (__bridge CFDictionaryRef)pixelAttributes, &rawPool); > > Even if it is safe to assume it is OK to ignore this failing, we should log > an error when it does. Will do. Comment on attachment 372372 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=372372&action=review >>> Source/WebCore/platform/graphics/avfoundation/objc/ImageDecoderAVFObjC.mm:428 >>> + if (m_imageRotationSession) >> >> Might be worth adding ASSERT(m_imageRotationSession) here to catch this in debug builds. > > No, this is fine: if the movie and track transforms are both identity, then we won't create a rotation session. Yes, but don’t you return early if it has a identity transform? Created attachment 372469 [details]
Patch for landing
Comment on attachment 372469 [details] Patch for landing Clearing flags on attachment: 372469 Committed r246611: <https://trac.webkit.org/changeset/246611> All reviewed patches have been landed. Closing bug. The new test media/video-orientation-canvas.html added in https://trac.webkit.org/changeset/246611/webkit is a flakey failure. History: https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=media%2Fvideo-orientation-canvas.html Diff: https://build.webkit.org/results/Apple%20High%20Sierra%20Debug%20WK2%20(Tests)/r246632%20(8257)/media/video-orientation-canvas-diffs.html Looks like fringing around the white areas of the rendered image. This test should be considered a failure only if the image diff is very significant; I.e., only if the image rendered isn’t rotated at all. Is there a way to mark pixel tests as needing >1% diff to be considered a failure? Alternatively, we can re-write the tests so that we sample the rendered image at a specific pixel that should have a known color value, and test that it’s approximately the same color. Is there any movement on fixing this test? If more time is needed then we will roll this out until it can be fixed. *** Bug 199113 has been marked as a duplicate of this bug. *** Reverted r246611 for reason: Introduced a flakey test. Committed r246699: <https://trac.webkit.org/changeset/246699> Created attachment 372786 [details]
Patch for landing
Comment on attachment 372786 [details] Patch for landing Clearing flags on attachment: 372786 Committed r246759: <https://trac.webkit.org/changeset/246759> All reviewed patches have been landed. Closing bug. |