Bug 223850

Summary: REGRESSION(r268176): [GStreamer] media/video-orientation-canvas.html fails
Product: WebKit Reporter: Philippe Normand <pnormand>
Component: PlatformAssignee: Philippe Normand <pnormand>
Status: RESOLVED FIXED    
Severity: Normal CC: calvaris, cgarcia, clopez, eric.carlson, ews-watchlist, glenn, gns, jer.noble, lmoura, menard, peng.liu6, philipj, sergio, vjaquez, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=95299
https://bugs.webkit.org/show_bug.cgi?id=224064
Attachments:
Description Flags
Patch clopez: review+

Description Philippe Normand 2021-03-28 02:52:32 PDT
--- /app/webkit/WebKitBuild/Release/layout-test-results/media/video-orientation-canvas-expected.txt
+++ /app/webkit/WebKitBuild/Release/layout-test-results/media/video-orientation-canvas-actual.txt
@@ -1,6 +1,6 @@

-EXPECTED (context.getImageData(88, 115, 1, 1).data == [208,0,0,255] with tolerance 5 OK
+EXPECTED (context.getImageData(88, 115, 1, 1).data == [208,0,0,255] with tolerance 5, OBSERVED '[0,0,0,255]' FAIL                                                                                                
 EXPECTED (context.getImageData(237, 88, 1, 1).data == [208,0,0,255] with tolerance 5 OK
-EXPECTED (context.getImageData(200, 237, 1, 1).data == [208,0,0,255] with tolerance 5 OK
+EXPECTED (context.getImageData(200, 237, 1, 1).data == [208,0,0,255] with tolerance 5, OBSERVED '[0,0,0,255]' FAIL                                                                                               
 END OF TEST
Comment 1 Philippe Normand 2021-03-28 05:28:28 PDT
Created attachment 424493 [details]
Patch
Comment 2 Peng Liu 2021-03-30 16:13:30 PDT
Comment on attachment 424493 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=424493&action=review

> Source/WebCore/ChangeLog:9
> +        handling of the image rotation tags is now performed only the pipeline is not able to do it

Nit. s/only/only when/g.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:2775
> +            m_shouldHandleOrientationTags = true;

Do we need to have a branch to set `m_shouldHandleOrientationTags` to false?
Comment 3 Philippe Normand 2021-03-31 01:15:38 PDT
Comment on attachment 424493 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=424493&action=review

>> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:2775
>> +            m_shouldHandleOrientationTags = true;
> 
> Do we need to have a branch to set `m_shouldHandleOrientationTags` to false?

No, the variable is initialized to false already and only one pipeline is created per MediaPlayerPrivate.
Comment 4 Carlos Alberto Lopez Perez 2021-04-01 13:47:23 PDT
*** Bug 224064 has been marked as a duplicate of this bug. ***
Comment 5 Carlos Alberto Lopez Perez 2021-04-01 13:48:34 PDT
On r275376 I marked this test as failing, please remove the failed expectation once this lands.
Comment 6 Carlos Alberto Lopez Perez 2021-04-01 13:51:46 PDT
BTW.. I think the regression was caused by r274076 and not by r268176 (as the title implies)
Comment 7 Peng Liu 2021-04-01 13:58:50 PDT
(In reply to Carlos Alberto Lopez Perez from comment #6)
> BTW.. I think the regression was caused by r274076 and not by r268176 (as
> the title implies)

Oh? But r274076 is a change to Cocoa ports only.
Comment 8 Philippe Normand 2021-04-01 14:16:40 PDT
(In reply to Carlos Alberto Lopez Perez from comment #6)
> BTW.. I think the regression was caused by r274076 and not by r268176 (as
> the title implies)

I found r268176 with results.webkit.org, though I admit I haven't tried a local revert to verify it.
Comment 9 Carlos Alberto Lopez Perez 2021-04-01 14:17:48 PDT
(In reply to Peng Liu from comment #7)
> (In reply to Carlos Alberto Lopez Perez from comment #6)
> > BTW.. I think the regression was caused by r274076 and not by r268176 (as
> > the title implies)
> 
> Oh? But r274076 is a change to Cocoa ports only.

Ups.. yes.

You're right.

I got confused because r274076 showed as the first revision making this test fail on the bot result history.
But that was just because r274076 removed this test as expected failure from LayoutTests/platform/wk2/TestExpectations

So, ignore my comment. Sorry for the noise
Comment 10 Carlos Alberto Lopez Perez 2021-04-01 14:23:06 PDT
Comment on attachment 424493 [details]
Patch

Patch looks fine to me.
I have tested it works and fixes the test.
Please remove the expected failure from GTK TestExpectations file when landing it.
Comment 11 Philippe Normand 2021-04-02 01:55:45 PDT
Committed r275412 (236075@main): <https://commits.webkit.org/236075@main>
Comment 12 Radar WebKit Bug Importer 2021-04-02 01:56:15 PDT
<rdar://problem/76142996>