Bug 196772 - iOS 12.2 Drawing portrait video to canvas is sideways
Summary: iOS 12.2 Drawing portrait video to canvas is sideways
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Canvas (show other bugs)
Version: Safari 12
Hardware: iPhone / iPad iOS 12
: P2 Normal
Assignee: Jer Noble
URL:
Keywords: InRadar
: 199113 (view as bug list)
Depends on:
Blocks:
 
Reported: 2019-04-10 09:44 PDT by Doug Parker
Modified: 2019-06-24 12:57 PDT (History)
10 users (show)

See Also:


Attachments
The top part has the video playing correctly, while the bottom is drawn on a canvas sideways. (625.08 KB, image/png)
2019-04-10 09:44 PDT, Doug Parker
no flags Details
Bug reproducing in production Bulletin (403.25 KB, image/png)
2019-04-10 09:44 PDT, Doug Parker
no flags Details
Patch (106.80 KB, patch)
2019-06-17 15:18 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-highsierra (3.18 MB, application/zip)
2019-06-17 16:25 PDT, Build Bot
no flags Details
Patch (106.87 KB, patch)
2019-06-17 16:33 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-highsierra (3.31 MB, application/zip)
2019-06-17 17:13 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews104 for mac-highsierra-wk2 (2.90 MB, application/zip)
2019-06-17 17:35 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews113 for mac-highsierra (3.14 MB, application/zip)
2019-06-17 18:00 PDT, Build Bot
no flags Details
Patch (114.49 KB, patch)
2019-06-18 13:28 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Patch for landing (115.91 KB, patch)
2019-06-19 10:03 PDT, Jer Noble
no flags Details | Formatted Diff | Diff
Patch for landing (115.94 KB, patch)
2019-06-24 12:26 PDT, Jer Noble
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Doug Parker 2019-04-10 09:44:10 PDT
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.
Comment 1 Doug Parker 2019-04-10 09:44:44 PDT
Created attachment 367131 [details]
Bug reproducing in production Bulletin
Comment 2 Doug Parker 2019-04-10 09:47:46 PDT
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.
Comment 3 Radar WebKit Bug Importer 2019-04-10 10:55:47 PDT
<rdar://problem/49781802>
Comment 4 Jer Noble 2019-06-06 10:00:53 PDT
The bug also reproduces on Desktop Safari with a portrait video taken on iOS.
Comment 5 Jer Noble 2019-06-06 10:18:14 PDT
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.
Comment 6 Jer Noble 2019-06-17 15:18:03 PDT
Created attachment 372278 [details]
Patch
Comment 7 Build Bot 2019-06-17 16:25:33 PDT
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
Comment 8 Build Bot 2019-06-17 16:25:34 PDT
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
Comment 9 Jer Noble 2019-06-17 16:33:15 PDT
Created attachment 372291 [details]
Patch
Comment 10 Build Bot 2019-06-17 16:35:41 PDT
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 11 Build Bot 2019-06-17 17:13:22 PDT
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
Comment 12 Build Bot 2019-06-17 17:13:23 PDT
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 13 Build Bot 2019-06-17 17:35:30 PDT
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
Comment 14 Build Bot 2019-06-17 17:35:31 PDT
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 15 Build Bot 2019-06-17 18:00:20 PDT
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
Comment 16 Build Bot 2019-06-17 18:00:22 PDT
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
Comment 17 Jer Noble 2019-06-18 13:03:54 PDT
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.
Comment 18 Jer Noble 2019-06-18 13:28:36 PDT
Created attachment 372372 [details]
Patch
Comment 19 Eric Carlson 2019-06-18 14:11:53 PDT
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.
Comment 20 Jer Noble 2019-06-18 14:38:20 PDT
(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 21 Eric Carlson 2019-06-18 18:52:50 PDT
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?
Comment 22 Jer Noble 2019-06-19 10:03:55 PDT
Created attachment 372469 [details]
Patch for landing
Comment 23 WebKit Commit Bot 2019-06-19 14:33:42 PDT
Comment on attachment 372469 [details]
Patch for landing

Clearing flags on attachment: 372469

Committed r246611: <https://trac.webkit.org/changeset/246611>
Comment 24 WebKit Commit Bot 2019-06-19 14:33:44 PDT
All reviewed patches have been landed.  Closing bug.
Comment 26 Jer Noble 2019-06-20 10:07:31 PDT
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?
Comment 27 Jer Noble 2019-06-20 10:08:21 PDT
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.
Comment 28 Truitt Savell 2019-06-21 11:17:16 PDT
Is there any movement on fixing this test? If more time is needed then we will roll this out until it can be fixed.
Comment 29 Russell Epstein 2019-06-21 11:27:38 PDT
*** Bug 199113 has been marked as a duplicate of this bug. ***
Comment 30 Truitt Savell 2019-06-21 15:09:05 PDT
Reverted r246611 for reason:

Introduced a flakey test.

Committed r246699: <https://trac.webkit.org/changeset/246699>
Comment 31 Jer Noble 2019-06-24 12:26:29 PDT
Created attachment 372786 [details]
Patch for landing
Comment 32 WebKit Commit Bot 2019-06-24 12:57:52 PDT
Comment on attachment 372786 [details]
Patch for landing

Clearing flags on attachment: 372786

Committed r246759: <https://trac.webkit.org/changeset/246759>
Comment 33 WebKit Commit Bot 2019-06-24 12:57:54 PDT
All reviewed patches have been landed.  Closing bug.