Closed Bug 1228601 Opened 9 years ago Closed 8 years ago

Video rotation metadata is not taken into account when playing back the video directly in the browser

Categories

(Core :: Audio/Video: Playback, defect, P2)

Product:
Core
Component:
Audio/Video: Playback
Version:
42 Branch
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla49
Tracking Flags:
Tracking Status
firefox49 --- fixed

People

(Reporter: naicuoctavian, Assigned: kikuo)

References

(
URL
)

Details

(Keywords: testcase, Whiteboard: [parity-chrome][parity-safari][parity-edge])

Attachments

(6 files, 3 obsolete files)

Screen-Shot-2015-06-10-at-16.48.07.png
33.78 KB, image/png
Details
Rotated video playback in Firefox 42 on Mac
321.81 KB, image/png
Details
comparison.png
295.86 KB, image/png
Details
MozReview Request: Bug 1228601 - [Part1] Store only supported video rotation informatin into VideoInfo.; r=mattwoodrow
58 bytes, text/x-review-board-request
mattwoodrow
: review+
Details
MozReview Request: Bug 1228601 - [Part2] Swap width,height if necessary and apply rotation matrix while building layer.; r=mattwoodrow
58 bytes, text/x-review-board-request
kikuo
: review+
Details
MozReview Request: Bug 1228601 - [Part3] Add reftest to check rotation by <video> and capture the result after playback ended; r?mattwoodrow
58 bytes, text/x-review-board-request
mattwoodrow
: review+
Details 
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/46.0.2490.86 Safari/537.36

Steps to reproduce:

1.Record a video with a mobile device in portrait mode (90 degrees or 270 degrees) or landcape (180 degrees).
2.Play it back in the browser on any desktop.


Actual results:

Video is not rotated properly, it is shown flipped on the side or upside down depending on the way it was recorded.


Expected results:

The video should playback with the correct orientation.

I am attaching an image for reference to see exactly the way the rotation is perceived by Android and iOS devices.
Component: Untriaged → Audio/Video: Playback
Product: Firefox → Core
The issue can be tested with this video which ahs been recorded from a mobile device:
https://addpipevideos.s3.amazonaws.com/29d9277c9b566c0e2322ce4540f36f5d/mvs285869022.mp4

It's rotation metadata is 90.
The above video is shown properly (as portrait) by Chrome, Safari, Edge. on Firefox is shown as landscape.

The issue is visible with both Firefox 42 on both Mac and PC.
Hardware: Unspecified → All
The below linked video file (portrait video) as played by Firefox 42 on Mac
Attached image comparison.png — 
Difference between how Chrome and Firefox play the same mp4 file containing rotation metadata (90 degrees).
Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:45.0) Gecko/20100101 Firefox/45.0
20151127030231

Thank you for the update. This is reproducible in the current Nightly.

(In reply to naicuoctavian from comment #2)
> The above video is shown properly (as portrait) by Chrome, Safari, Edge.

Opera 33 displays it the same as Firefox, while in IE11 it doesn't seem possible to open video files directly. I'm not able to test the other browsers, so I'm taking your word for it.
URL: https://addpipevideos.s3.amazonaws.co...
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: testcase
OS: Unspecified → All
Whiteboard: [parity-chrome][parity-safari][parity-edge]
Here a random web-developer using nightly…
I face this issue on a regular basis with my application, as our user base is narrow enough, we were able to make them use Firefox (so it ease my development).

The bug reporter didn't mentioned  it (even if he is aware of it ;-)) but, some stack overflow threads are talking about this bug.

http://stackoverflow.com/questions/29920331/video-orientation-is-incorrect-on-firefox/31416252#31416252
>in IE11 it doesn't seem possible to open video files directly

I'm pasting here a quick snippet to test on IE11 and similar browsers:

<video controls>
<source src="https://addpipevideos.s3.amazonaws.com/29d9277c9b566c0e2322ce4540f36f5d/mvs285869022.mp4" type="video/mp4"/>
</video>

test it on 

http://www.w3schools.com/html/tryit.asp?filename=tryhtml_intro

@Christophe Narbonne

Indeed the issue is the subject of a few SO threads. It still needs to be fixed though.
Assignee: nobody → giles
Priority: -- → P2
Are there any plans to fix this in Firefox.
Basically all browsers support it already https://addpipe.com/blog/mp4-rotation-metadata-in-mobile-video-files/
@alesrebec
It looks like an accepted bug with an assigned developer and a priority set, so I suppose it'll be fixed soon, yet the reporting date is not so old…
I also have found this issue within Firefox browser. Please fix this. This is a major problem for our users as well using the Firefox browser. 

Here is an example of the same issue with our videos within Firefox. This was recorded in an Android Note 4, portrait mode. 

our Video example: 

https://www.selocial.com/share/56bbc9baf2d42802421e4f1d


Thank you for your help.
Is there a workaround until this is fixed ?
(In reply to Antoine Turmel [:GeekShadow] from comment #11)
> Is there a workaround until this is fixed ?

You could probably work around the issue using a CSS transform.

Jean-Yves - this issue is quite literally a pain in the neck. I'd like to get this issue resolved but it is obviously not as important as the stuff you're currently working on.
Flags: needinfo?(jyavenard)
Would need to amend the VideoData and VideoInfo to include a "rotation" parameter and the compositor to know how to deal with those.

In any case, the majority of the work is to be done by the compositor.

One issue remain is what to use for the display resolution? do we rotate them first or would gfx able to handle all of this.

Matt, any suggestions?
Flags: needinfo?(jyavenard) → needinfo?(matt.woodrow)
I don't think it overly matters, you can spec it however you'd like.

You'll need to find someone to implement it though, it's a non-trivial amount of work.
Flags: needinfo?(matt.woodrow)
UH what? wouldn't you be able to do it by the following week? I'm shocked
I'm shocked that this is still not solved!
It is working in other major browsers, it should be working in Firefox too.
You're holding your screen wrong when watching those iOS videos. Turn your head 90 degres either to the left or right :)
Personally, I've always promoted Firefox to my app users, but since I added videos upload and found this bug, even if I prefer Firefox, I have to suggest to use chrome instead.
(In reply to alesrebec from comment #16)
> I'm shocked that this is still not solved!

As Matt explained in c14 it is a non-trivial amount of work, which is not always obvious when you're not directly involved.

Blake - can you find someone to do the media playback side of this?

Matt - is the rest of the orientations stuff a graphics or layout job?
Flags: needinfo?(matt.woodrow)
Flags: needinfo?(bwu)
I assume orientation is generally a constant for the video, not something we care about per-frame.

It would make sense to put it into nsDisplayVideo::BuildLayer (layout) I think, similar to how we handle aspect ratio.
Flags: needinfo?(matt.woodrow)
After discussing offline, Kilik can help on media playback part.
Flags: needinfo?(bwu)
VideoRotation information is parsed [1], but is not updated into MP4VideoInfo [2], I'll provide a patch fixing this.
[1] https://dxr.mozilla.org/mozilla-central/source/media/libstagefright/frameworks/av/media/libstagefright/MPEG4Extractor.cpp#2189
[2] https://dxr.mozilla.org/mozilla-central/source/media/libstagefright/binding/DecoderData.cpp#160


Is it suitable for ImageContainer providing a API like ImageContainer::SetScaleHit [3], so that we can pass the rotation information from media to layers ? 

[3] https://dxr.mozilla.org/mozilla-central/source/gfx/layers/ImageContainer.h#498
Flags: needinfo?(matt.woodrow)
I think we need to update layout, not gfx about the rotation.

Layout is going to be working using the un-rotated size, so gfx can either ignore the size that layout expected (bad), or try fit the rotated video within the unrotated rectangle (it'll be tiny).

You probably want to fix nsVideoFrame::GetVideoIntrinsicSize to provide it with the information that width and height are going to be swapped.

You also want to make nsVideoFrame::BuildLayer include the rotation transform.
Flags: needinfo?(matt.woodrow)
(In reply to Matt Woodrow (:mattwoodrow) from comment #23)
> I think we need to update layout, not gfx about the rotation.
> 
> Layout is going to be working using the un-rotated size, so gfx can either
> ignore the size that layout expected (bad), or try fit the rotated video
> within the unrotated rectangle (it'll be tiny).
> 
> You probably want to fix nsVideoFrame::GetVideoIntrinsicSize to provide it
> with the information that width and height are going to be swapped.
> 
> You also want to make nsVideoFrame::BuildLayer include the rotation
> transform.

If the video rotation degree is 180, it's easy to calculate a rotation matrix in nsVideoFrame::BuildLayer to fix it. Now I'm trying to handle 90/270 cases, and I'm starting to figure out this part ( nsVideoFrame::GetVideoIntrinsicSize). Thanks for the suggestions : )
Hi Ralph, 
I've fixed this issue at my local pc, would like to push for review, do you mind that I'm taking this bug ?
Flags: needinfo?(giles)
No, that's great. Please take the bug. :)
Assignee: giles → kikuo
Flags: needinfo?(giles)
(In reply to Ralph Giles (:rillian) needinfo me from comment #26)
> No, that's great. Please take the bug. :)

Thanks :)
Comment on attachment 8750567 [details]
MozReview Request: Bug 1228601 - [Part1] Store only supported video rotation informatin into VideoInfo.; r=mattwoodrow

https://reviewboard.mozilla.org/r/51335/#review48293

::: dom/media/MediaInfo.h:279
(Diff revision 1)
>    RefPtr<MediaByteBuffer> mCodecSpecificConfig;
>    RefPtr<MediaByteBuffer> mExtraData;
>  
> +  // Describing how many degrees video frames should be rotated in clock-wise to
> +  // get correct view.
> +  uint32_t mRotationDegrees;

Rather than a uint32_t, can this instead be an enum with the 4 valid rotation positions?

It looks like the stagefright only ever outputs these values, so we should enforce that as early on as possible.
Attachment #8750567 - Flags: review?(matt.woodrow)
Comment on attachment 8750568 [details]
MozReview Request: Bug 1228601 - [Part2] Set video rotation information into ImageContainer.; r?mattwoodrow

https://reviewboard.mozilla.org/r/51337/#review48295

HMTLMediaElement has a a MediaInfo member which contains VideoInfo.

Can't we just use that to access the rotation value on VideoInfo, and then we don't need to store it on the ImageContainer at all?
Attachment #8750568 - Flags: review?(matt.woodrow)
Attachment #8750569 - Flags: review?(matt.woodrow)
Comment on attachment 8750569 [details]
MozReview Request: Bug 1228601 - [Part3] Handle video rotation while building layer from nsVideoFrame.; r?mattwoodrow

https://reviewboard.mozilla.org/r/51339/#review48297

::: layout/generic/nsVideoFrame.cpp:158
(Diff revision 1)
>  {
>    return true;
>  }
>  
> +Matrix
> +nsVideoFrame::RotationByVideoInfo(gfxFloat aWidth, gfxFloat aHeight, uint32_t aDegrees)

This function doesn't use VideoInfo, maybe 'ComputeRotationMatrix' would be better?

It also could probably just be a local static function rather than a member

::: layout/generic/nsVideoFrame.cpp:221
(Diff revision 1)
> +  uint32_t rotationDegrees = container->GetRotationDegrees();
> +  if (rotationDegrees == 90 || rotationDegrees == 270) {
> +    scaleHint.width = static_cast<int32_t>(destGFXRect.Height());
> +    scaleHint.height = static_cast<int32_t>(destGFXRect.Width());
> +  } else {
> +    scaleHint.width = static_cast<int32_t>(destGFXRect.Width());
> +    scaleHint.height = static_cast<int32_t>(destGFXRect.Height());
> +  }

I think you should be performing the swap on 'frameSize' rather than 'scaleHint' here.
Also, should we only be taking the requested rotation into account when the video is played directly? It's not obvious that we should be rotating it when it's embedded as a <video>.
(In reply to Matt Woodrow (:mattwoodrow) from comment #31)
> It looks like the stagefright only ever outputs these values, so we should
> enforce that as early on as possible.

We shouldn't design around stagefright because it is up for replacement. That also means that having a test (e.g. a reftest) would be much appreciated.
(In reply to Anthony Jones (:kentuckyfriedtakahe, :k17e) from comment #35)
 
> We shouldn't design around stagefright because it is up for replacement.
> That also means that having a test (e.g. a reftest) would be much
> appreciated.

Agreed. We want the 4 fixed rotation positions we want to support, not the integer number of degrees that stagefright outputs.
https://reviewboard.mozilla.org/r/51335/#review48293

> Rather than a uint32_t, can this instead be an enum with the 4 valid rotation positions?
> 
> It looks like the stagefright only ever outputs these values, so we should enforce that as early on as possible.

Yes I think so, there's similar definition for webrtc, like [1]. I'll define one enum in MediaInfo.

[1] https://dxr.mozilla.org/mozilla-central/source/media/webrtc/trunk/webrtc/common_video/rotation.h#17
https://reviewboard.mozilla.org/r/51337/#review48295

Cool! I'll fix it, thanks for the suggestion
https://reviewboard.mozilla.org/r/51339/#review48297

> I think you should be performing the swap on 'frameSize' rather than 'scaleHint' here.

Sorry, I don't quite understand here. 'frameSize' here is only used for checking the existence of image.
Please correct me if I'm wrong, if we want width/height in 'frameSize' swapped, which means the image size in ImageContainer::mCurrentImages is also swapped.  To achieve this, we need to make sure width/height is swapped in the aConfig at the very begining where the VideoDecoder is created. Because this videoInfo will be passed into VideoDecoder as configuration and be used for creating VideoData. Besides we might also need to swap the w/h in parsed SPSData to make the mDisplay consistent.
Also, I'm not sure whether there's a certain VideoDecoder requiring exact width/height for decoding content instead of requiring a total size (w*h) for buffer allocation.

If I understand the coce correctly, the destGFXRect is the final rect to place the content. The 'scaleHint' is used to tell gfx scaling the content to desired w/h. So I tried to compute the rotation matrix here which indicates I'd like to treat the video content alwasy in a 0-rotation-degree at first, and then leveraging the render command for translation & rotation.
That's why I was treating the video frameSize (e.g. always 1280x700) as it comes from metadata (regardless of the rotation, then do rotate/scale while building layer. If this is an acceptable solution, I realized that I should also need to correct the return values in HTMLVideoElement::VideoWidth(), HTMLVideoElement::VideoHeight().
If this is not an acceptable solution, should the solution be the one mentioned in 1st paragraph ?

Hopefully I explained it clearly. I'm still trying to figure out the position between layers and media here.
(In reply to Kilik Kuo [:kikuo] from comment #39)
> https://reviewboard.mozilla.org/r/51339/#review48297
> 
> > I think you should be performing the swap on 'frameSize' rather than 'scaleHint' here.
> 
> Sorry, I don't quite understand here. 'frameSize' here is only used for
> checking the existence of image.
> Please correct me if I'm wrong, if we want width/height in 'frameSize'
> swapped, which means the image size in ImageContainer::mCurrentImages is
> also swapped.  To achieve this, we need to make sure width/height is swapped
> in the aConfig at the very begining where the VideoDecoder is created.
> Because this videoInfo will be passed into VideoDecoder as configuration and
> be used for creating VideoData. Besides we might also need to swap the w/h
> in parsed SPSData to make the mDisplay consistent.
> Also, I'm not sure whether there's a certain VideoDecoder requiring exact
> width/height for decoding content instead of requiring a total size (w*h)
> for buffer allocation.
> 
> If I understand the coce correctly, the destGFXRect is the final rect to
> place the content. The 'scaleHint' is used to tell gfx scaling the content
> to desired w/h. So I tried to compute the rotation matrix here which
> indicates I'd like to treat the video content alwasy in a 0-rotation-degree
> at first, and then leveraging the render command for translation & rotation.
> That's why I was treating the video frameSize (e.g. always 1280x700) as it
> comes from metadata (regardless of the rotation, then do rotate/scale while
> building layer. If this is an acceptable solution, I realized that I should
> also need to correct the return values in HTMLVideoElement::VideoWidth(),
> HTMLVideoElement::VideoHeight().
> If this is not an acceptable solution, should the solution be the one
> mentioned in 1st paragraph ?
> 
> Hopefully I explained it clearly. I'm still trying to figure out the
> position between layers and media here.

Sorry, I misread this code, I think what you currently have is fine.

'dest' should be in the rotated coordinate space (since 'videoSizeInPx' and 'area' were), but the scale hint we provide to the container is in un-rotated coordinate space.

You're swapping the width/height back (if necessary) so that 'scaleHint' is unrotated.

Can you just add a comment to that effect please? It might also be nice to have a 'SwapWidthHeightForRotation' or similar helper for the few places we need to do this.
https://reviewboard.mozilla.org/r/51339/#review48297

> Sorry, I don't quite understand here. 'frameSize' here is only used for checking the existence of image.
> Please correct me if I'm wrong, if we want width/height in 'frameSize' swapped, which means the image size in ImageContainer::mCurrentImages is also swapped.  To achieve this, we need to make sure width/height is swapped in the aConfig at the very begining where the VideoDecoder is created. Because this videoInfo will be passed into VideoDecoder as configuration and be used for creating VideoData. Besides we might also need to swap the w/h in parsed SPSData to make the mDisplay consistent.
> Also, I'm not sure whether there's a certain VideoDecoder requiring exact width/height for decoding content instead of requiring a total size (w*h) for buffer allocation.
> 
> If I understand the coce correctly, the destGFXRect is the final rect to place the content. The 'scaleHint' is used to tell gfx scaling the content to desired w/h. So I tried to compute the rotation matrix here which indicates I'd like to treat the video content alwasy in a 0-rotation-degree at first, and then leveraging the render command for translation & rotation.
> That's why I was treating the video frameSize (e.g. always 1280x700) as it comes from metadata (regardless of the rotation, then do rotate/scale while building layer. If this is an acceptable solution, I realized that I should also need to correct the return values in HTMLVideoElement::VideoWidth(), HTMLVideoElement::VideoHeight().
> If this is not an acceptable solution, should the solution be the one mentioned in 1st paragraph ?
> 
> Hopefully I explained it clearly. I'm still trying to figure out the position between layers and media here.

I dont think this is right from the decoder point of view, and will surely not work when we have a pixel aspect ratio different than 1:1. We check the decoded dimensions as returned by the decoder and adjust them based on what we find in the metadata. the decoder will return that the video size is 700x1280 but the metadata we have now say 1280x720, thats going to be giving weird results.

i think its important that the dimensions are kept like the originals and like what the decoded video will be all the way until it needs to be drawn.
Comment on attachment 8750567 [details]
MozReview Request: Bug 1228601 - [Part1] Store only supported video rotation informatin into VideoInfo.; r=mattwoodrow

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51335/diff/1-2/
Attachment #8750567 - Attachment description: MozReview Request: Bug 1228601 - [Part1] Store video rotation informatin into VideoInfo.; r?mattwoodrow → MozReview Request: Bug 1228601 - [Part1] Store only supported video rotation informatin into VideoInfo.; r?mattwoodrow
Attachment #8750567 - Flags: review?(matt.woodrow)
Attachment #8750568 - Attachment is obsolete: true
Attachment #8750569 - Attachment is obsolete: true
I should provide a reftest like the test for bug917595.  But I need to find out a suitable case first.
Comment on attachment 8750567 [details]
MozReview Request: Bug 1228601 - [Part1] Store only supported video rotation informatin into VideoInfo.; r=mattwoodrow

https://reviewboard.mozilla.org/r/51335/#review48991

::: dom/media/MediaInfo.h:279
(Diff revision 2)
> +        return kDegree_90;
> +      case 180:
> +        return kDegree_180;
> +      case 270:
> +        return kDegree_270;
> +      default:

Add a NS_WARNING here (if aDegree != 0) that an invalid rotation was specified and was ignored.
Attachment #8750567 - Flags: review?(matt.woodrow) → review+
Attachment #8751386 - Flags: review?(matt.woodrow)
Comment on attachment 8751386 [details]
MozReview Request: Bug 1228601 - [Part2] Swap width,height if necessary and apply rotation matrix while building layer.; r?mattwoodrow

https://reviewboard.mozilla.org/r/51989/#review48995

Should we be doing this rotation for <video> as well as playing the video directly? The bug as filed only concerned the latter.

What do other browsers do for that? Does the spec say anything?

Needs a test too.
(In reply to Matt Woodrow (:mattwoodrow) from comment #46)
> Comment on attachment 8751386 [details]
> MozReview Request: Bug 1228601 - [Part2] Swap width,height if necessary and
> apply rotation matrix while building layer.; r?mattwoodrow
> 
> https://reviewboard.mozilla.org/r/51989/#review48995
> 
> Should we be doing this rotation for <video> as well as playing the video
> directly? The bug as filed only concerned the latter.
> 
> What do other browsers do for that? Does the spec say anything?
> 
> Needs a test too.

Never mind, I checked this myself, they do the rotation for <video> too.
https://reviewboard.mozilla.org/r/51989/#review48997

Please add tests before landing this (of both playing a video directly, and <video>)
Comment on attachment 8751386 [details]
MozReview Request: Bug 1228601 - [Part2] Swap width,height if necessary and apply rotation matrix while building layer.; r?mattwoodrow

https://reviewboard.mozilla.org/r/51989/#review48999
Attachment #8751386 - Flags: review+
Comment on attachment 8750567 [details]
MozReview Request: Bug 1228601 - [Part1] Store only supported video rotation informatin into VideoInfo.; r=mattwoodrow

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51335/diff/2-3/
Attachment #8751386 - Attachment is obsolete: true
https://reviewboard.mozilla.org/r/51335/#review48991

> Add a NS_WARNING here (if aDegree != 0) that an invalid rotation was specified and was ignored.

Thanks, addressed !
Comment on attachment 8751862 [details]
MozReview Request: Bug 1228601 - [Part2] Swap width,height if necessary and apply rotation matrix while building layer.; r=mattwoodrow

Weird, I imported this patch from review board, and push it w/o modification.
Carry r+ from Comment 49.
Attachment #8751862 - Flags: review?(matt.woodrow) → review+
https://reviewboard.mozilla.org/r/52249/#review49165

::: dom/html/reftests/reftest.list:61
(Diff revision 1)
>  
>  # Test imageset is using permissions.default.image
>  pref(permissions.default.image,1) HTTP == bug1196784-with-srcset.html bug1196784-no-srcset.html
>  pref(permissions.default.image,2) HTTP == bug1196784-with-srcset.html bug1196784-no-srcset.html
> +
> +fuzzy(2,49898) == bug1228601-video-rotation-90.html bug1228601-video-rotated-90-ref.html

I don't know how to dump the rotated video from firefox. So I compared the rotated result frome firefox and from rotated video/png generated by ffmpeg. Please let me know if I'm doing it in right way.

The best quality I get for directly playing is max difference = 150, max differing pixels = 5295. Though there's barely difference can be noticed by eyes.

bug1228601-video-rotation-90.mp4 ==> Recorded and transcoded video with rotation:90.
bug1228601-video-rotated-90.png ==> Rotated frame extracted by ffmpeg.
===
(generated by ffmpeg)
$>ffmpeg -ss 0.0 -i bug1228601-video-rotation-90.mp4 -t 1 -f image2 bug1228601-video-rotated-90.png
===


bug1228601-video-rotation-0.mp4 ==> Rotated video with rotation:0
===
(generated by ffmpeg)
$>ffmpeg -i bug1228601-video-rotation-90.mp4 -metadata:s:v rotate="0" -crf 0 bug1228601-video-rotation-0.mp4
===
Attachment #8751863 - Flags: review?(matt.woodrow)
Comment on attachment 8751863 [details]
MozReview Request: Bug 1228601 - [Part3] Add reftest to check rotation by <video> and capture the result after playback ended; r?mattwoodrow

https://reviewboard.mozilla.org/r/52249/#review49289

I can't get bug1228601-video-rotation-0.mp4 to work locally, it appears to be corrupt.

::: dom/html/reftests/reftest.list:62
(Diff revision 1)
>  # Test imageset is using permissions.default.image
>  pref(permissions.default.image,1) HTTP == bug1196784-with-srcset.html bug1196784-no-srcset.html
>  pref(permissions.default.image,2) HTTP == bug1196784-with-srcset.html bug1196784-no-srcset.html
> +
> +fuzzy(2,49898) == bug1228601-video-rotation-90.html bug1228601-video-rotated-90-ref.html
> +fuzzy(150,5295) == bug1228601-video-rotation-90.mp4 bug1228601-video-rotation-0.mp4

Why is there such a large fuzz value here?
https://reviewboard.mozilla.org/r/52249/#review49289

Those created .mp4 files contain only 1 frame, it won't be played correctly on VLC player.
You should be albe to see the frame by dragging them into browser (i.e. FF / Chrome) or open it by Totme Videos.

> Why is there such a large fuzz value here?

I think that was resulted from the encoding when I created these MP4 files from still PNGs (content image is complicated inside).
I reproduced new test clips with simple color pattern inside and can be tested without using fuzzy().  Hooray !
Comment on attachment 8751863 [details]
MozReview Request: Bug 1228601 - [Part3] Add reftest to check rotation by <video> and capture the result after playback ended; r?mattwoodrow

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52249/diff/1-2/
Attachment #8751863 - Flags: review?(matt.woodrow)
Comment on attachment 8751863 [details]
MozReview Request: Bug 1228601 - [Part3] Add reftest to check rotation by <video> and capture the result after playback ended; r?mattwoodrow

https://reviewboard.mozilla.org/r/52249/#review49586
Attachment #8751863 - Flags: review?(matt.woodrow) → review+
(In reply to Matt Woodrow (:mattwoodrow) from comment #59)
> Comment on attachment 8751863 [details]
> MozReview Request: Bug 1228601 - [Part3] Add 2 reftests for video rotation
> of both directly playing and <video>.; r?mattwoodrow
> 
> https://reviewboard.mozilla.org/r/52249/#review49586

Thanks for the review : )
try run, https://treeherder.mozilla.org/#/jobs?repo=try&revision=21e188228640&selectedJob=20885734

Uh, the test failed on "OSX 10.10 opt" & "Android 4.3 API15+ opt", but not on debug environment nor Linux / Windows.
...
http://hg.mozilla.org/mozilla-central/raw-file/tip/layout/tools/reftest/reftest-analyzer.xhtml#logurl=http://archive.mozilla.org/pub/firefox/try-builds/kikuo@mozilla.com-21e1882286400495c8293aad5d790a5e1f0a6953/try-macosx64/try_yosemite_r7_test-reftest-bm132-tests1-macosx-build27.txt.gz&only_show_unexpected=1
...

The difference is caused by the icon "X" and the wording "video can't be played", but it's rotated correctly.
I'll need to find out how to avoid that.
(In reply to Kilik Kuo [:kikuo] from comment #62)
> The difference is caused by the icon "X" and the wording "video can't be
> played", but it's rotated correctly.
> I'll need to find out how to avoid that.

I checked the clips with Firefox on Windows / Linux / Mac, it won't be decoded correctly on Windows/Mac.
But with Chrome on those platforms, it's decoded correctly.

Turns out I found ffmpeg would avoid color subsampling when creating clip via libx264. It seems not well supported for those non-ffmpeg-based players. [1].

[1] https://trac.ffmpeg.org/wiki/Create%20a%20video%20slideshow%20from%20images#Colorspaceconversionandchromasub-sampling

So I re-generated 2 clips with pixel format yuv420p, and decoding correctly on Mac/Windows.

I'll rebase these patches and post the try result. Only clips are re-generated, I think no review is needed.
Comment on attachment 8750567 [details]
MozReview Request: Bug 1228601 - [Part1] Store only supported video rotation informatin into VideoInfo.; r=mattwoodrow

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51335/diff/3-4/
Attachment #8750567 - Attachment description: MozReview Request: Bug 1228601 - [Part1] Store only supported video rotation informatin into VideoInfo.; r?mattwoodrow → MozReview Request: Bug 1228601 - [Part1] Store only supported video rotation informatin into VideoInfo.; r=mattwoodrow
Attachment #8751862 - Attachment description: MozReview Request: Bug 1228601 - [Part2] Swap width,height if necessary and apply rotation matrix while building layer.; r?mattwoodrow → MozReview Request: Bug 1228601 - [Part2] Swap width,height if necessary and apply rotation matrix while building layer.; r=mattwoodrow
Attachment #8751863 - Attachment description: MozReview Request: Bug 1228601 - [Part3] Add 2 reftests for video rotation of both directly playing and <video>.; r?mattwoodrow → MozReview Request: Bug 1228601 - [Part3] Add 2 reftests for video rotation of both directly playing and <video>.; r=mattwoodrow
Attachment #8751862 - Flags: review+ → review?(matt.woodrow)
Comment on attachment 8751862 [details]
MozReview Request: Bug 1228601 - [Part2] Swap width,height if necessary and apply rotation matrix while building layer.; r=mattwoodrow

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52247/diff/1-2/
Comment on attachment 8751863 [details]
MozReview Request: Bug 1228601 - [Part3] Add reftest to check rotation by <video> and capture the result after playback ended; r?mattwoodrow

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52249/diff/2-3/
Comment on attachment 8751862 [details]
MozReview Request: Bug 1228601 - [Part2] Swap width,height if necessary and apply rotation matrix while building layer.; r=mattwoodrow

Carry r+ from comment 49/54
Attachment #8751862 - Flags: review?(matt.woodrow) → review+
(In reply to Kilik Kuo [:kikuo] from comment #68)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=e24dbccd53bf
> 
> New try, looks great for now :)

Except the jobs on Windows 8 which have been pending for 16 hours, others are all green.
I think it's good to go !
Keywords: checkin-needed
(In reply to Sebastian H. [:aryx][:archaeopteryx] from comment #71)
> Backed out for reftest failure in 1frame_rotation_90.mp4:
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/
> c040302a52dc8c09a71944a6e0076e375e2d3630
> https://hg.mozilla.org/integration/mozilla-inbound/rev/
> 1597a092668da4ce163d2a56b1103ca432591eef
> https://hg.mozilla.org/integration/mozilla-inbound/rev/
> 2a623cf3774735b84a2936d4e2fd089b04482c4f
> 
> Push with shows these failures (previous runs were busted because of a
> different commit):
> https://treeherder.mozilla.org/#/jobs?repo=mozilla-
> inbound&revision=d3d23c5640717bfb9c72db8e951c462685991854
  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
I checked this result, found ...
1. On Linux, all passed
2. On Mac, OSX10-dbg, failed when directly playing.[1]
3. On WinXP, all failed (file load failed: null)
   On Win7, all passed
   On Win8(dbg), failed when directly playing [2]
   On Win8(pgo), failed when using <video> tag. [3]

AFAIK (from MDN), the snapshot for reftest is done after the 'load' event for document has been fired.
But the document's initial painting may not be finished.  Also, in this case, the 1st video frame may be decoded but not be rendered.

For cases like [3], I should delay the snapshot by adding 'class="reftest-wait"' in *.html and remove it after video playback finished.
But for cases like [1]/[2], I don't know if there's a way to control the timing of snapshot because no script/attribute can be added. I also found no directly video playing reftest even for (mp4)[4], (ogg)[5], (webm)[6]. So I'm wondering if the directly video playing test is necessary ?
Matt, any suggestions ?


[1] http://hg.mozilla.org/mozilla-central/raw-file/tip/layout/tools/reftest/reftest-analyzer.xhtml#logurl=http://archive.mozilla.org/pub/firefox/tinderbox-builds/mozilla-inbound-macosx64-debug/1463996320/mozilla-inbound_yosemite_r7-debug_test-reftest-bm136-tests1-macosx-build100.txt.gz&only_show_unexpected=1
[2] http://hg.mozilla.org/mozilla-central/raw-file/tip/layout/tools/reftest/reftest-analyzer.xhtml#logurl=http://archive.mozilla.org/pub/firefox/tinderbox-builds/mozilla-inbound-win64-debug/1463996320/mozilla-inbound_win8_64-debug_test-reftest-bm111-tests1-windows-build1204.txt.gz&only_show_unexpected=1
[3] http://hg.mozilla.org/mozilla-central/raw-file/tip/layout/tools/reftest/reftest-analyzer.xhtml#logurl=http://archive.mozilla.org/pub/firefox/tinderbox-builds/mozilla-inbound-win64-pgo/1464003014/mozilla-inbound_win8_64_test_pgo-reftest-bm109-tests1-windows-build68.txt.gz&only_show_unexpected=1

[4] https://dxr.mozilla.org/mozilla-central/source/layout/reftests/mp4-video/reftest.list
[5] https://dxr.mozilla.org/mozilla-central/source/layout/reftests/ogg-video/reftest.list
[6] https://dxr.mozilla.org/mozilla-central/source/layout/reftests/webm-video/reftest.list
Flags: needinfo?(kikuo) → needinfo?(matt.woodrow)
(In reply to Kilik Kuo [:kikuo] from comment #72)
>
> But the document's initial painting may not be finished.  Also, in this
> case, the 1st video frame may be decoded but not be rendered.
> 

For the loadeddata event to be fired, the image would have been decoded and passed to the compositor.

JW can tell you more.

We have plenty of reftest checking that the image is painted prior loadeddata and that we can also read back from the container.

To get permafail here would indicate another problem than what you describe I would think.
Ok, that's somewhat unfortunate.

It would be nice to have proper tests for this, we might need to extend the reftest harness to either know about media files, or to allow the reftest.list format to specify events to wait on.

Could you please file a bug to get tests added for playing videos directly and attach your test to that?

I think it's ok to land this without the extra test in the meantime.
Flags: needinfo?(matt.woodrow)
(In reply to Jean-Yves Avenard [:jya] from comment #73) 
> For the loadeddata event to be fired, the image would have been decoded and
> passed to the compositor.
> 
> JW can tell you more.
> 
> We have plenty of reftest checking that the image is painted prior
> loadeddata and that we can also read back from the container.
> 
> To get permafail here would indicate another problem than what you describe
> I would think.

The failing test is loading a .mp4 file as the main document, not embedding it within a <video> tag.

The reftest harness only waits for "load", not "loadeddata" (since it doesn't know anything about video specific events), and we capture a snapshot before the video has loaded.

The normal process for getting the reftest harness for wait for other things doesn't work here, since we can't put JS script within the .mp4 file.
Thanks Matt, Bug 1276457 is created as a follow-up for reftest on direct media playback.

New try with controlling script, 
"""
+<!DOCTYPE html>
+<html class="reftest-wait">
+<body>
+<script>
+  function done() {
+    document.documentElement.removeAttribute("class");
+  }
+</script>
+  <video src="video_rotated.mp4" onended="done()" autoplay>
+</body>
+</html>
"""

Try run, https://treeherder.mozilla.org/#/jobs?repo=try&revision=241eba35b207,
Looks good on all platforms except Android (failure : timed out waiting for reftest-wait to be removed)
That's pretty weird, I've done local reftest on Linux with "--repeat 1500", no failure happened.
Will try hooking on different event.
Comment on attachment 8750567 [details]
MozReview Request: Bug 1228601 - [Part1] Store only supported video rotation informatin into VideoInfo.; r=mattwoodrow

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51335/diff/4-5/
Attachment #8751863 - Attachment description: MozReview Request: Bug 1228601 - [Part3] Add 2 reftests for video rotation of both directly playing and <video>.; r=mattwoodrow → MozReview Request: Bug 1228601 - [Part3] Add reftest to check rotation by <video> and capture the result after playback ended; r?mattwoodrow
Attachment #8751862 - Flags: review+ → review?(matt.woodrow)
Comment on attachment 8751862 [details]
MozReview Request: Bug 1228601 - [Part2] Swap width,height if necessary and apply rotation matrix while building layer.; r=mattwoodrow

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52247/diff/2-3/
Comment on attachment 8751863 [details]
MozReview Request: Bug 1228601 - [Part3] Add reftest to check rotation by <video> and capture the result after playback ended; r?mattwoodrow

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52249/diff/3-4/
Attachment #8751862 - Flags: review+
Comment on attachment 8751862 [details]
MozReview Request: Bug 1228601 - [Part2] Swap width,height if necessary and apply rotation matrix while building layer.; r=mattwoodrow

https://reviewboard.mozilla.org/r/52247/#review52792

Not changed, carry r+ from Comment 49, Comment 54
Comment on attachment 8751862 [details]
MozReview Request: Bug 1228601 - [Part2] Swap width,height if necessary and apply rotation matrix while building layer.; r=mattwoodrow

Remove the r? triggered from mozreview. This patch is already r+ but not tracked by mozreivew (See Comment 49)
Attachment #8751862 - Flags: review?(matt.woodrow)
Keywords: checkin-needed
Works good on latest nightly on Ubuntu/Linux x86_64. Thumbs up !
Thumbs up!+1
Works great on my macbook pro(10.11.5) with latest nightly.
Depends on: 1446175
See Also: → 1423850
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: