1243538 - webm videos are not working any more with new WebMDemuxer

Status: VERIFIED FIXED

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

Description

[r.rudolph]

Attachment: 2016-01-27-225722_1024x768_scrot.png

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux i686; rv:44.0) Gecko/20100101 Firefox/44.0
Build ID: 20160125134036

Steps to reproduce:

visit:
https://bug821503.bmoattachments.org/attachment.cgi?id=692296



Actual results:

In Linux/Lubuntu the video is not working.


Expected results:

In Windows the video should run.
Some months ago, on older versions of Firefox the video was working in Linux/Lubuntu.

Comment 1

[[:philipp]]

i can confirm that the video runs fine on win7/win10

Comment 2

[Mardeg]

Confirming with the reporter on IRC that my i686 Linux (Slacko Puppy in my case) doesn't play WebM files natively or within a <video> while media.format-reader.webm is set to true.

We both set it to false and the attached testcase worked and I also tested a number of other WebM files to reproduce this.

Comment 3

[Mardeg]

My previous comment is about Firefox 44.

I just tested in the latest Nightly build and found not only is the video working, but the media.format-reader.webm pref doesn't exist in about:config

Not sure what the story is with that pref.

Comment 4

[Mardeg]

Reporter's about:support

Grafik
------

Asynchrones Wischen und Zoomen: nichts
Gerätekennung: Mesa DRI nv17 x86/MMX+/3DNow!+/SSE
GPU-beschleunigte Fenster: 0/1 Basic (OMTC) Wurde auf Grund Ihrer Grafikkarte blockiert, da ungelöste Treiberprobleme bestehen.
H264-Hardware-Dekodierung unterstützt: No;
Herstellerkennung: Nouveau
Karten-Beschreibung: Nouveau -- Mesa DRI nv17 x86/MMX+/3DNow!+/SSE
Treiber-Version: 1.2 Mesa 10.1.3
WebGL-Renderer: Wurde auf Grund Ihrer Grafikkarte blockiert, da ungelöste Treiberprobleme bestehen.
windowLayerManagerRemote: true
AzureCanvasBackend: cairo
AzureContentBackend: cairo
AzureFallbackCanvasBackend: none
AzureSkiaAccelerated: 0
CairoUseXRender: 1

My about:support

Graphics
--------

Adapter Description: X.Org R300 Project -- Gallium 0.4 on ATI RV350
Asynchronous Pan/Zoom: none
Device ID: Gallium 0.4 on ATI RV350
Driver Version: 2.1 Mesa 8.0.4
GPU Accelerated Windows: 0/1 Basic (OMTC)
Supports Hardware H264 Decoding: No;
Vendor ID: X.Org R300 Project
WebGL Renderer: X.Org R300 Project -- Gallium 0.4 on ATI RV350
windowLayerManagerRemote: true
AzureCanvasBackend: cairo
AzureContentBackend: cairo
AzureFallbackCanvasBackend: none
AzureSkiaAccelerated: 0
CairoUseXRender: 1

Comment 5

[Jean-Yves Avenard [:jya]]

(In reply to Mardeg from comment #3)
> My previous comment is about Firefox 44.
> 
> I just tested in the latest Nightly build and found not only is the video
> working, but the media.format-reader.webm pref doesn't exist in about:config
> 

this pref no longer exists because the old WebMReader code is no longer available from version 45 onward

> Not sure what the story is with that pref.

There are several ways that your problem may have been fixed and are some suggestions on how to resolve your problem:
1- Install Firefox 46 (Developer Edition) which includes a very recent version of ffvp8 and ffvp9
2- Update to a newer version of FFmpeg (you haven't mentioned which version libavcodec is installed unfortunately)
3- Disable ffmpeg with media.ffmpeg.enabled = false (in about:config) to revert to using libvpx

h264/aac/mp4 decoding will be done via gstreamer once again.

FWIW, I can't reproduce the issue with either ubuntu 12.04 (which ships with LibAV 0.8, we use libvpx there), or 14.04 with LibAV 9 (libavcodec 54)
nor FFmpeg 2.8 or FFmpeg master.

So a bit at loss on why a very simply vp8/vorbis video wouldn't play

Comment 6

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Is the issue reproducible in nightly?

Comment 7

[aburton1889]

bug also happens in arm linux (android) the fix is to set the boolean mentioned above in about:config to false.

Comment 8

[r.rudolph]

In Firefox 45.0 in Linux/Lubuntu this bug does not appear any more.

Comment 9

[r.rudolph]

Sorry, but my last comment (#8) is wrong.
After I reseted Firefox 45.0, the bug still exists.

Comment 10

[Jean-Yves Avenard [:jya]]

Which version of ffmpeg do you have installed? Do you have more than one version of ffmpeg installed?

Comment 11

[Mardeg]

ffmpeg -version
says 2.0

Comment 12

[Mardeg]

I can't tell if more than one version is installed.
other info:
libavcodec 55.18.102
libavformat 55.12.100

Comment 13

[feroz]

I am currently seeing this bug on Firefox 45.0.1 on Windows 8.1 and Firefox 44 on Ubuntu Vivid.

Changing the value of "media.format-reader.webm" to 'false' using the about:config page fixed the issue in Windows. I haven't tried the same on Ubuntu yet.

Comment 14

[Jean-Yves Avenard [:jya]]

(In reply to feroz from comment #13)
> I am currently seeing this bug on Firefox 45.0.1 on Windows 8.1 and Firefox
> 44 on Ubuntu Vivid.

Please provide a link to a webm video not working.

Comment 15

[feroz]

(In reply to Jean-Yves Avenard [:jya] from comment #14)
> (In reply to feroz from comment #13)
> > I am currently seeing this bug on Firefox 45.0.1 on Windows 8.1 and Firefox
> > 44 on Ubuntu Vivid.
> 
> Please provide a link to a webm video not working.

http://365.argh.in/images/25.webm

Comment 16

[Jean-Yves Avenard [:jya]]

can reproduce with this last sample

Comment 17

[Jean-Yves Avenard [:jya]]

Attachment: MozReview Request: Bug 1243538: P1. Make MediaInfo::mImage an nsIntSize again and introduce a mImageRect member. r?mattwoodrow

This member contains the original size of the image, uncropped. Required for webm container to pass to the VPx decoder.

Review commit: https://reviewboard.mozilla.org/r/46653/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/46653/

Comment 18

[Jean-Yves Avenard [:jya]]

Attachment: MozReview Request: Bug 1243538: P2. Add convenience VideoInfo::ScaledImageRect. r?mattwoodrow

Review commit: https://reviewboard.mozilla.org/r/46655/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/46655/

Comment 19

[Jean-Yves Avenard [:jya]]

Attachment: MozReview Request: Bug 1243538: P3. Adjust libvpx decoder to allow different decoding size from metadata. r?mattwoodrow

Review commit: https://reviewboard.mozilla.org/r/46657/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/46657/

Comment 20

[Jean-Yves Avenard [:jya]]

Attachment: MozReview Request: Bug 1243538: P4. Adjust ffvpx decoder to allow different decoding size from metadata. r?mattwoodrow

Review commit: https://reviewboard.mozilla.org/r/46659/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/46659/

Comment 21

[Jean-Yves Avenard [:jya]]

rillian, will need to amend the rust mp4 demuxer to update the VideoInfo object accordingly.

I've placed an assert in the ffmpeg decoder that VideoInfo::mImage == VideoInfo.mOriginalImage

Matt, the other approach would be to stop storing mImage as the crop value, but instead have mImage be a nsIntSize again, and have a nsIntRect with cropping value instead.

maybe more elegant than having to similar mImage

Comment 22

[Matt Woodrow (:mattwoodrow)]

(In reply to Jean-Yves Avenard [:jya] from comment #21)
> rillian, will need to amend the rust mp4 demuxer to update the VideoInfo
> object accordingly.
> 
> I've placed an assert in the ffmpeg decoder that VideoInfo::mImage ==
> VideoInfo.mOriginalImage
> 
> Matt, the other approach would be to stop storing mImage as the crop value,
> but instead have mImage be a nsIntSize again, and have a nsIntRect with
> cropping value instead.
> 
> maybe more elegant than having to similar mImage

Yeah, I prefer this latter approach.

Could we have mSize for the size reported by the container, and mPictureRect (to match what we call it all through VideoData and layers) for the crop rect?

It would be nice to put the code for rescaling the picture/crop rect into a shared place too.

Comment 23

[Jean-Yves Avenard [:jya]]

Comment on attachment 8741623 [details]
MozReview Request: Bug 1243538: P1. Make MediaInfo::mImage an nsIntSize again and introduce a mImageRect member. r?mattwoodrow

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46653/diff/1-2/

Comment 24

[Jean-Yves Avenard [:jya]]

Comment on attachment 8741624 [details]
MozReview Request: Bug 1243538: P2. Add convenience VideoInfo::ScaledImageRect. r?mattwoodrow

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46655/diff/1-2/

Comment 25

[Jean-Yves Avenard [:jya]]

Comment on attachment 8741625 [details]
MozReview Request: Bug 1243538: P3. Adjust libvpx decoder to allow different decoding size from metadata. r?mattwoodrow

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46657/diff/1-2/

Comment 26

[Jean-Yves Avenard [:jya]]

Comment on attachment 8741626 [details]
MozReview Request: Bug 1243538: P4. Adjust ffvpx decoder to allow different decoding size from metadata. r?mattwoodrow

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46659/diff/1-2/

Comment 27

[Jean-Yves Avenard [:jya]]

Comment on attachment 8741624 [details]
MozReview Request: Bug 1243538: P2. Add convenience VideoInfo::ScaledImageRect. r?mattwoodrow

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46655/diff/2-3/

Comment 28

[Jean-Yves Avenard [:jya]]

Comment on attachment 8741625 [details]
MozReview Request: Bug 1243538: P3. Adjust libvpx decoder to allow different decoding size from metadata. r?mattwoodrow

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46657/diff/2-3/

Comment 29

[Jean-Yves Avenard [:jya]]

Comment on attachment 8741626 [details]
MozReview Request: Bug 1243538: P4. Adjust ffvpx decoder to allow different decoding size from metadata. r?mattwoodrow

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46659/diff/2-3/

Comment 30

[Jean-Yves Avenard [:jya]]

Attachment: MozReview Request: Bug 1243538: P6. Adjust gonk decoder to allow different decoding size from metadata. r?alfredo

We let the WMF decoder calculate the decoded image size, scaling the picture region (cropping) from the original metadata.
As only webm defines cropping in its metadata, the scaled picture region will always be identical to the image size when decoding h264.

Review commit: https://reviewboard.mozilla.org/r/46753/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/46753/

Comment 31

[Chris Pearce [:cpearce (Not reading bugmail)]]

What was the problem that caused the video to stop playing?

Comment 32

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8741793 [details]
MozReview Request: Bug 1243538: P6. Adjust gonk decoder to allow different decoding size from metadata. r?alfredo

https://reviewboard.mozilla.org/r/46753/#review43521

::: dom/media/platforms/wmf/WMFVideoMFTManager.cpp:406
(Diff revision 1)
>    hr = mediaType->GetGUID(MF_MT_SUBTYPE, &videoFormat);
>    NS_ENSURE_TRUE(videoFormat == MFVideoFormat_NV12 || !mUseHwAccel, E_FAIL);
>    NS_ENSURE_TRUE(videoFormat == MFVideoFormat_YV12 || mUseHwAccel, E_FAIL);
>  
> -  UINT32 width = mVideoInfo.mImage.width;
> -  UINT32 height = mVideoInfo.mImage.height;
> +  nsIntRect pictureRegion;
> +  hr = GetPictureRegion(mediaType, pictureRegion);

Does this regress bug 1217226?

Comment 33

[Jean-Yves Avenard [:jya]]

(In reply to Chris Pearce (:cpearce) from comment #31)
> What was the problem that caused the video to stop playing?

The picture size in the metadata is greater than the actual decoded size, causing the image to be refused due to IsValidVideoRegiom returning false. The old WebMReader would accept those, and instead rescale the cropping rectangle to the new decoding size.

This patch follows the same logic.

Comment 34

[Jean-Yves Avenard [:jya]]

https://reviewboard.mozilla.org/r/46753/#review43521

> Does this regress bug 1217226?

No.

bug 1217226 was due to the metadata display size not being used (as we relied on the decoder to guess those).

This change is about the decoded picture size. we now always rely on the decoder to determine it, and the rescale the cropping window accordingly. this is the same logic used across all decoders.

i had manually tested that bug 1217226 (which is how I discovered that D3D11 had regressed the intel vp decoder) along the several other aspect ratio bugs.

Comment 35

[Jean-Yves Avenard [:jya]]

Comment on attachment 8741793 [details]
MozReview Request: Bug 1243538: P6. Adjust gonk decoder to allow different decoding size from metadata. r?alfredo

not sure mozreview handle only asking question.. but just to be sure it doesn't get lost

Comment 36

[Chris Pearce [:cpearce (Not reading bugmail)]]

https://reviewboard.mozilla.org/r/46655/#review43535

::: dom/media/MediaInfo.h:241
(Diff revision 3)
>    }
>  
> +  nsIntRect ScaledImageRect(int64_t aWidth, int64_t aHeight) const
> +  {
> +    if (aWidth == mImage.width && aHeight == mImage.height) {
> +      return ImageRect();

Don't you want to be returning ImageRect(aWidth, aHeight) here so the callers don't need to special case the case where no scaling is requried?

Comment 37

[Jean-Yves Avenard [:jya]]

https://reviewboard.mozilla.org/r/46655/#review43535

> Don't you want to be returning ImageRect(aWidth, aHeight) here so the callers don't need to special case the case where no scaling is requried?

No you don't (and there's no ImageRect(aWidth, aHeight): ImageRect() will return the non-scaled cropping window by default. ImageRect() is strictly equivalent to ScaledImageRect(mImage.width, mImage.height) which is the cropping window with no scaling

I made the use of accessors for the mImageRect member so I wouldn't have to update all the video demuxers and metadata handler (in particular the rust mp4metadata). It's more elegant too.

Comment 38

[Jean-Yves Avenard [:jya]]

Attachment: MozReview Request: Bug 1243538: [webm] P7. Let the decoder handle picture resizing. r?SingingTree

Is is now up to the decoders to automatically determine the correct size of the decoded frame.
By leaving the original dimensions in the VideoInfo, it will allow automatic rescale of the cropping window according to the original dimensions.

Review commit: https://reviewboard.mozilla.org/r/46959/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/46959/

Comment 39

[Jean-Yves Avenard [:jya]]

Comment on attachment 8741623 [details]
MozReview Request: Bug 1243538: P1. Make MediaInfo::mImage an nsIntSize again and introduce a mImageRect member. r?mattwoodrow

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46653/diff/2-3/

Comment 40

[Jean-Yves Avenard [:jya]]

Comment on attachment 8741624 [details]
MozReview Request: Bug 1243538: P2. Add convenience VideoInfo::ScaledImageRect. r?mattwoodrow

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46655/diff/3-4/

Comment 41

[Jean-Yves Avenard [:jya]]

Comment on attachment 8741625 [details]
MozReview Request: Bug 1243538: P3. Adjust libvpx decoder to allow different decoding size from metadata. r?mattwoodrow

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46657/diff/3-4/

Comment 42

[Jean-Yves Avenard [:jya]]

Comment on attachment 8741626 [details]
MozReview Request: Bug 1243538: P4. Adjust ffvpx decoder to allow different decoding size from metadata. r?mattwoodrow

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46659/diff/3-4/

Comment 43

[Jean-Yves Avenard [:jya]]

Comment on attachment 8741793 [details]
MozReview Request: Bug 1243538: P6. Adjust gonk decoder to allow different decoding size from metadata. r?alfredo

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46753/diff/1-2/

Comment 44

[Jean-Yves Avenard [:jya]]

Attachment: MozReview Request: Bug 1243538: P5. Adjust wmf decoder to allow different decoding size from metadata. r?cpearce

We let the WMF decoder calculate the decoded image size, scaling the picture region (cropping) from the original metadata.
As only webm defines cropping in its metadata, the scaled picture region will always be identical to the image size when decoding h264.

Review commit: https://reviewboard.mozilla.org/r/46961/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/46961/

Comment 45

[Jean-Yves Avenard [:jya]]

Comment on attachment 8741793 [details]
MozReview Request: Bug 1243538: P6. Adjust gonk decoder to allow different decoding size from metadata. r?alfredo

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46753/diff/2-3/

Comment 46

[Jean-Yves Avenard [:jya]]

Comment on attachment 8742163 [details]
MozReview Request: Bug 1243538: [webm] P7. Let the decoder handle picture resizing. r?SingingTree

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46959/diff/1-2/

Comment 47

[Jean-Yves Avenard [:jya]]

sorry for the noise, I moved fix for bug 1154162 here, as really this belong as a block

Comment 49

[Alfredo Yang (:alfredo)]

Comment on attachment 8741793 [details]
MozReview Request: Bug 1243538: P6. Adjust gonk decoder to allow different decoding size from metadata. r?alfredo

https://reviewboard.mozilla.org/r/46753/#review43541

LGTM

Comment 50

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8742164 [details]
MozReview Request: Bug 1243538: P5. Adjust wmf decoder to allow different decoding size from metadata. r?cpearce

https://reviewboard.mozilla.org/r/46961/#review43551

::: dom/media/platforms/wmf/WMFVideoMFTManager.h:69
(Diff revision 1)
>    bool CanUseDXVA(IMFMediaType* aType);
>  
>    // Video frame geometry.
>    VideoInfo mVideoInfo;
>    uint32_t mVideoStride;
> +  nsIntSize mVideoSize;

Can you please call this something more descriptive of what it's for, i.e. mFrameSize. "Video" could mean anything here.

::: dom/media/platforms/wmf/WMFVideoMFTManager.cpp:526
(Diff revision 1)
> +  nsIntRect pictureRegion =
> +    mVideoInfo.ScaledImageRect(videoWidth, videoHeight);
>    VideoData::SetVideoDataToImage(image,
>                                   mVideoInfo,
>                                   b,
> -                                 mVideoInfo.ImageRect(),
> +    pictureRegion,

Indentation is off here.

Comment 51

[Jean-Yves Avenard [:jya]]

https://reviewboard.mozilla.org/r/46961/#review43553

::: dom/media/platforms/wmf/WMFVideoMFTManager.h:69
(Diff revision 1)
>    bool CanUseDXVA(IMFMediaType* aType);
>  
>    // Video frame geometry.
>    VideoInfo mVideoInfo;
>    uint32_t mVideoStride;
> +  nsIntSize mVideoSize;

VideoBlah is what the rest of the code is using:
We have VideoStride, VideoWitdh, VideoHeight etc

So it felt logical to use that name in the context of the existing code.

Comment 52

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Comment on attachment 8742164 [details]
MozReview Request: Bug 1243538: P5. Adjust wmf decoder to allow different decoding size from metadata. r?cpearce

https://reviewboard.mozilla.org/r/46961/#review43557

I think these changes break dom/media/test/test_resolution_change.html Since we no longer have the resolution information being exposed at that level I think that test should be removed (it could be replaced with another test that can test the inner workings, but I'm unfamiliar with what we have in that area).

Comment 53

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Comment on attachment 8742163 [details]
MozReview Request: Bug 1243538: [webm] P7. Let the decoder handle picture resizing. r?SingingTree

https://reviewboard.mozilla.org/r/46959/#review43561

I think these changes break dom/media/test/test_resolution_change.html Since we no longer have the resolution information being exposed at that level I think that test should be removed (it could be replaced with another test that can test the inner workings, but I'm unfamiliar with what we have in that area).

Comment 54

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

https://reviewboard.mozilla.org/r/46961/#review43557

Argh, wrong review, my bad.

Comment 55

[Jean-Yves Avenard [:jya]]

https://reviewboard.mozilla.org/r/46961/#review43557

It should have passed the test. The resolution from the frames actually output doesn't change and this should be reflected in the element's width/height being updated. Going to test what's going on there.
If you think it will fail, then r- is appropriate :)

Comment 56

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to Jean-Yves Avenard [:jya] from comment #51)
> https://reviewboard.mozilla.org/r/46961/#review43553
> 
> ::: dom/media/platforms/wmf/WMFVideoMFTManager.h:69
> (Diff revision 1)
> >    bool CanUseDXVA(IMFMediaType* aType);
> >  
> >    // Video frame geometry.
> >    VideoInfo mVideoInfo;
> >    uint32_t mVideoStride;
> > +  nsIntSize mVideoSize;
> 
> VideoBlah is what the rest of the code is using:
> We have VideoStride, VideoWitdh, VideoHeight etc
> 
> So it felt logical to use that name in the context of the existing code.

That's *precisely* why it's confusing!

Comment 57

[Jean-Yves Avenard [:jya]]

https://reviewboard.mozilla.org/r/46959/#review43631

::: dom/media/webm/WebMDemuxer.cpp
(Diff revision 2)
>              (si.w != mLastSeenFrameWidth.value() ||
>               si.h != mLastSeenFrameHeight.value())) {
> -          // We ignore cropping information on resizes during streams.
> -          // Cropping alone is rare, and we do not consider cropping to
> -          // still be valid after a resolution change
> -          mInfo.mVideo.mImage = mInfo.mVideo.mDisplay = nsIntSize(si.w, si.h);

oh actually, my bad, I incorrectly removed the change to mDisplay...

Comment 58

[Jean-Yves Avenard [:jya]]

Comment on attachment 8741623 [details]
MozReview Request: Bug 1243538: P1. Make MediaInfo::mImage an nsIntSize again and introduce a mImageRect member. r?mattwoodrow

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46653/diff/3-4/

Comment 59

[Jean-Yves Avenard [:jya]]

Comment on attachment 8741624 [details]
MozReview Request: Bug 1243538: P2. Add convenience VideoInfo::ScaledImageRect. r?mattwoodrow

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46655/diff/4-5/

Comment 60

[Jean-Yves Avenard [:jya]]

Comment on attachment 8741625 [details]
MozReview Request: Bug 1243538: P3. Adjust libvpx decoder to allow different decoding size from metadata. r?mattwoodrow

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46657/diff/4-5/

Comment 61

[Jean-Yves Avenard [:jya]]

Comment on attachment 8741626 [details]
MozReview Request: Bug 1243538: P4. Adjust ffvpx decoder to allow different decoding size from metadata. r?mattwoodrow

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46659/diff/4-5/

Comment 62

[Jean-Yves Avenard [:jya]]

Comment on attachment 8742164 [details]
MozReview Request: Bug 1243538: P5. Adjust wmf decoder to allow different decoding size from metadata. r?cpearce

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46961/diff/1-2/

Comment 63

[Jean-Yves Avenard [:jya]]

Comment on attachment 8741793 [details]
MozReview Request: Bug 1243538: P6. Adjust gonk decoder to allow different decoding size from metadata. r?alfredo

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46753/diff/3-4/

Comment 64

[Jean-Yves Avenard [:jya]]

Comment on attachment 8742163 [details]
MozReview Request: Bug 1243538: [webm] P7. Let the decoder handle picture resizing. r?SingingTree

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46959/diff/2-3/

Comment 65

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

https://reviewboard.mozilla.org/r/46959/#review43903

Looks good.

Comment 66

[Chris Pearce [:cpearce (Not reading bugmail)]]

jya: does this affect YouTube? If so we'll need to uplift this to 46.

Comment 67

[Maire Reavy [:mreavy]]

We may "wontfix" this, but it depends on what jya says about the impact of this bug on YouTube.

Comment 68

[Maire Reavy [:mreavy]]

Per discussion with jya and cpearce in irc, this bug should not impact YouTube.  So we will "wontfix" this for Fx46 and plan to uplift the fix to Fx47 -- assuming the fix lands this week (during the Fx48 Nightly cycle).

Comment 69

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8741623 [details]
MozReview Request: Bug 1243538: P1. Make MediaInfo::mImage an nsIntSize again and introduce a mImageRect member. r?mattwoodrow

https://reviewboard.mozilla.org/r/46653/#review43963

Comment 70

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8741624 [details]
MozReview Request: Bug 1243538: P2. Add convenience VideoInfo::ScaledImageRect. r?mattwoodrow

https://reviewboard.mozilla.org/r/46655/#review43967

::: dom/media/MediaInfo.h:238
(Diff revision 5)
>    void SetImageRect(const nsIntRect& aRect)
>    {
>      mImageRect = aRect;
>    }
>  
> +  nsIntRect ScaledImageRect(int64_t aWidth, int64_t aHeight) const

Please document this function including why we need it, and where the input parameters come from.

Comment 71

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8741625 [details]
MozReview Request: Bug 1243538: P3. Adjust libvpx decoder to allow different decoding size from metadata. r?mattwoodrow

https://reviewboard.mozilla.org/r/46657/#review43969

Comment 72

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8741626 [details]
MozReview Request: Bug 1243538: P4. Adjust ffvpx decoder to allow different decoding size from metadata. r?mattwoodrow

https://reviewboard.mozilla.org/r/46659/#review43971

Comment 73

[Jean-Yves Avenard [:jya]]

Comment on attachment 8741623 [details]
MozReview Request: Bug 1243538: P1. Make MediaInfo::mImage an nsIntSize again and introduce a mImageRect member. r?mattwoodrow

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46653/diff/4-5/

Comment 74

[Jean-Yves Avenard [:jya]]

Comment on attachment 8741624 [details]
MozReview Request: Bug 1243538: P2. Add convenience VideoInfo::ScaledImageRect. r?mattwoodrow

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46655/diff/5-6/

Comment 75

[Jean-Yves Avenard [:jya]]

Comment on attachment 8741625 [details]
MozReview Request: Bug 1243538: P3. Adjust libvpx decoder to allow different decoding size from metadata. r?mattwoodrow

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46657/diff/5-6/

Comment 76

[Jean-Yves Avenard [:jya]]

Comment on attachment 8741626 [details]
MozReview Request: Bug 1243538: P4. Adjust ffvpx decoder to allow different decoding size from metadata. r?mattwoodrow

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46659/diff/5-6/

Comment 77

[Jean-Yves Avenard [:jya]]

Comment on attachment 8742164 [details]
MozReview Request: Bug 1243538: P5. Adjust wmf decoder to allow different decoding size from metadata. r?cpearce

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46961/diff/2-3/

Comment 78

[Jean-Yves Avenard [:jya]]

Comment on attachment 8741793 [details]
MozReview Request: Bug 1243538: P6. Adjust gonk decoder to allow different decoding size from metadata. r?alfredo

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46753/diff/4-5/

Comment 79

[Jean-Yves Avenard [:jya]]

Comment on attachment 8742163 [details]
MozReview Request: Bug 1243538: [webm] P7. Let the decoder handle picture resizing. r?SingingTree

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46959/diff/3-4/

Comment 80

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7ce9649c759f
https://hg.mozilla.org/integration/mozilla-inbound/rev/028595062225
https://hg.mozilla.org/integration/mozilla-inbound/rev/58cf5d2934fb
https://hg.mozilla.org/integration/mozilla-inbound/rev/f2c1da947a12
https://hg.mozilla.org/integration/mozilla-inbound/rev/9bcedb72a5c3
https://hg.mozilla.org/integration/mozilla-inbound/rev/0c19d2ae60ce
https://hg.mozilla.org/integration/mozilla-inbound/rev/1687a256328b

Comment 81

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/7ce9649c759f
https://hg.mozilla.org/mozilla-central/rev/028595062225
https://hg.mozilla.org/mozilla-central/rev/58cf5d2934fb
https://hg.mozilla.org/mozilla-central/rev/f2c1da947a12
https://hg.mozilla.org/mozilla-central/rev/9bcedb72a5c3
https://hg.mozilla.org/mozilla-central/rev/0c19d2ae60ce
https://hg.mozilla.org/mozilla-central/rev/1687a256328b

Comment 82

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hi Jean-Yves, Maire, is this fix stable and safe to uplift to Beta47 now? Or should we consider baking it some more in Nightly49 and Aurora48? Thanks!

Comment 83

[Jean-Yves Avenard [:jya]]

Only P1 to P6 are required if earlier than 48.

There will be some rebasing required, but I do believe the changes are safe as is...

Comment 84

[Maire Reavy [:mreavy]]

(In reply to Ritu Kothari (:ritu) from comment #82)
> Hi Jean-Yves, Maire, is this fix stable and safe to uplift to Beta47 now? Or
> should we consider baking it some more in Nightly49 and Aurora48? Thanks!

jya has answered already, but in the future you want to needinfo :k17e (Anthony Jones) as the eng manager for Audio/Video::Playback, not me.  (I handle many of the other pieces of media, including WebRTC, Web Audio, MSG, Recording, etc.)

Comment 85

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hi Anthony, this came up on my radar as a P1 regression that is fixed on Nightly and also affects Fx47. Do you think we should uplift this to Beta47? Jean-Yves believes that we should but I'd like a second opinion on whether it's risky and needs more bake time or not uplift at all. Thanks!

Comment 86

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

I'd rather uplift these patches. They aren't especially complicated.

Comment 87

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Thanks Anthony. Jean-Yves, could you please nominate relevant patches for uplift to Beta47?

Comment 88

[Jean-Yves Avenard [:jya]]

Comment on attachment 8741623 [details]
MozReview Request: Bug 1243538: P1. Make MediaInfo::mImage an nsIntSize again and introduce a mImageRect member. r?mattwoodrow

Approval Request Comment
[Feature/regressing bug #]:1243538
[User impact if declined]: some videos do not play
[Describe test coverage new/current, TreeHerder]:in central for a while, manual tests. 
[Risks and why]: low. Going back to the earlier behaviour 
[String/UUID change made/needed]: none

Comment 89

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8741623 [details]
MozReview Request: Bug 1243538: P1. Make MediaInfo::mImage an nsIntSize again and introduce a mImageRect member. r?mattwoodrow

WebM videos not working on some versions of Linux/Ubuntu. Baked in Nightly for 3 weeks, P1 regression (second opinion on risk evaluation was deemed safe and to uplift). Aurora48+, Beta47+

Comment 90

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hello Feroz, Mardeg, could you please verify this issue is fixed as expected on a latest Nightly build? Thanks!

Comment 91

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

This already landed during the 48 cycle, so aurora is covered. 

This doesn't apply to beta, we'll need new patches for at least part 1.

Comment 92

[Mardeg]

Confirming the URL field video at http://365.argh.in/images/25.webm does play in the latest Nightly and fails to play in 47.0b3

Comment 93

[feroz]

(In reply to Ritu Kothari (:ritu) from comment #90)
> Hello Feroz, Mardeg, could you please verify this issue is fixed as expected
> on a latest Nightly build? Thanks!

Can confirm this works for me as well in the latest Nightly (49.0a1). Thanks very much!

Comment 94

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

(In reply to feroz from comment #93)
> (In reply to Ritu Kothari (:ritu) from comment #90)
> > Hello Feroz, Mardeg, could you please verify this issue is fixed as expected
> > on a latest Nightly build? Thanks!
> 
> Can confirm this works for me as well in the latest Nightly (49.0a1). Thanks
> very much!

Awesome! Thank you so much for a prompt follow up.

Comment 95

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

(In reply to Mardeg from comment #92)
> Confirming the URL field video at http://365.argh.in/images/25.webm does
> play in the latest Nightly and fails to play in 47.0b3

Thank you for the verification! You should see the video working fine in 47.0b5 or b6 (which will include the fix).

Comment 96

[Jean-Yves Avenard [:jya]]

it was committed earlier:
a4a650440913	Jean-Yves Avenard — Bug 1243538: P6. Adjust gonk decoder to allow different decoding size from metadata. r=alfredo,a=ritu
2002cd5c8153	Jean-Yves Avenard — Bug 1243538: P5. Adjust wmf decoder to allow different decoding size from metadata. r=cpearce,a=ritu
8453a9b7de85	Jean-Yves Avenard — Bug 1243538: P4. Adjust ffvpx decoder to allow different decoding size from metadata. r=mattwoodrow,a=ritu
29c947854d0f	Jean-Yves Avenard — Bug 1243538: P3. Adjust libvpx decoder to allow different decoding size from metadata. r=mattwoodrow,a=ritu
e6b672d20c14	Jean-Yves Avenard — Bug 1243538: P2. Add convenience VideoInfo::ScaledImageRect. r=mattwoodrow,a=ritu
4d4b624adde7	Jean-Yves Avenard — Bug 1243538: P1. Make MediaInfo::mImage an nsIntSize again and introduce a mImageRect member. r=mattwoodrow,a=ritu

Comment 97

[Comorasu Cristian]

Reproduced this issue using Fx 46.0.1 on Ubuntu 14.04 x64, Windows 10 x64 and Mac OS X 10.9.5.
Confirming the fix on Firefox 47.0b6(20160516123243), 48.0a2 (20160517004009) and 49.0a1 (20160514030209) across platforms.

Comment 98

[prasanthp96]

Reproduced this issue using Firefox 48.0 b3  on windows 7 x64 .
Confirming the fix on Firefox 48.0 b3