Bug 196772 – iOS 12.2 Drawing portrait video to canvas is sideways

• Status: RESOLVED
• Resolution: FIXED
• Component: Canvas

[Doug Parker]

Created attachment 367130 [details]
The top part has the video playing correctly, while the bottom is drawn on a canvas sideways.

In iOS 12.2, using a canvas context to `drawImage()` of a <video /> element with a portrait-oriented source will render sideways, with the bottom cut off. Screenshots attached.

Demo: http://jsfiddle.net/3ugmLc0w/39/show
Source: http://jsfiddle.net/3ugmLc0w/39/

Visit the demo URL on iOS 12.2 (I have an iPod Touch, but we've seen this on other models), capture a test video (in portrait orientation) and play it. The <video /> tag will play correctly, but the <canvas /> will render sideways. Landscape videos seem to work ok.

We use this technique in our PWA to extract a thumbnail from a video by drawing the first frame onto a canvas, then saving that canvas to a Blob to store the thumbnail for use later. All of our portrait video thumbnails are now sideways do to this bug. This problem reproduces consistently and results in videos with very bad thumbnails. If you want to see this in our real app, the steps would be:
1) Visit https://posts.google.com/bulletin/app/ on an iOS 12.2 device.
2) Follow the steps to create an account until you get to the home screen.
3) Tap the floating action "Start Story" button in the bottom right.
4) Tap the "Take video" button and capture a portrait (!) video.
5) Observe that the video thumbnail shown is sideways.

Our only option for a workaround would be to detect iOS 12.2 and attempt to rotate the frame back to the correct orientation. This is clearly pretty hacky, and we'd rather avoid that if at all possible. If this bug can be limited to iOS 12.2 only via a timely WebKit fix, then client-side workarounds might not be necessary with iOS 12.3 on the way.

[Doug Parker]

Created attachment 367131 [details]
Bug reproducing in production Bulletin

[Doug Parker]

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.

[Radar WebKit Bug Importer]

<rdar://problem/49781802>

[Jer Noble]

The bug also reproduces on Desktop Safari with a portrait video taken on iOS.

[Jer Noble]

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.

[Jer Noble]

Created attachment 372278 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[Jer Noble]

Created attachment 372291 [details]
Patch

[EWS Watchlist]

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.

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[Jer Noble]

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.

[Jer Noble]

Created attachment 372372 [details]
Patch

[Eric Carlson]

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.

[Jer Noble]

(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.

[Eric Carlson]

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?

[Jer Noble]

Created attachment 372469 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 372469 [details]
Patch for landing

Clearing flags on attachment: 372469

Committed r246611: <https://trac.webkit.org/changeset/246611>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Truitt Savell]

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

[Jer Noble]

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?

[Jer Noble]

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.

[Truitt Savell]

Is there any movement on fixing this test? If more time is needed then we will roll this out until it can be fixed.

[Russell Epstein]

*** Bug 199113 has been marked as a duplicate of this bug. ***

[Truitt Savell]

Reverted r246611 for reason:

Introduced a flakey test.

Committed r246699: <https://trac.webkit.org/changeset/246699>

[Jer Noble]

Created attachment 372786 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 372786 [details]
Patch for landing

Clearing flags on attachment: 372786

Committed r246759: <https://trac.webkit.org/changeset/246759>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.