Closed Bug 1243538 Opened 8 years ago Closed 8 years ago

webm videos are not working any more with new WebMDemuxer

Categories

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

44 Branch
Unspecified
All
defect

Tracking

()

VERIFIED FIXED
mozilla48
Tracking Status
firefox45 --- wontfix
firefox46 --- wontfix
firefox47 --- fixed
firefox48 --- fixed
firefox49 --- verified

People

(Reporter: r.rudolph, Assigned: jya)

References

()

Details

(Keywords: regression)

Attachments

(8 files)

43.41 KB, image/png
Details
58 bytes, text/x-review-board-request
mattwoodrow
: review+
Details
58 bytes, text/x-review-board-request
mattwoodrow
: review+
Details
58 bytes, text/x-review-board-request
mattwoodrow
: review+
Details
58 bytes, text/x-review-board-request
mattwoodrow
: review+
Details
58 bytes, text/x-review-board-request
ayang
: review+
Details
58 bytes, text/x-review-board-request
bryce
: review+
Details
58 bytes, text/x-review-board-request
cpearce
: review+
bryce
: review+
Details
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.
i can confirm that the video runs fine on win7/win10
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: webm video are not working any more on Linux/Lubuntu → webm videos are not working any more on Linux/Lubuntu when media.format-reader.webm is set to true
Summary: webm videos are not working any more on Linux/Lubuntu when media.format-reader.webm is set to true → webm videos are not working any more on i686 Linux when media.format-reader.webm is set to true
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.
Component: Untriaged → Audio/Video: Playback
Product: Firefox → Core
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
(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
OS: Unspecified → Linux
Hardware: Unspecified → x86
Is the issue reproducible in nightly?
Flags: needinfo?(r.rudolph)
bug also happens in arm linux (android) the fix is to set the boolean mentioned above in about:config to false.
In Firefox 45.0 in Linux/Lubuntu this bug does not appear any more.
Flags: needinfo?(r.rudolph)
Sorry, but my last comment (#8) is wrong.
After I reseted Firefox 45.0, the bug still exists.
Which version of ffmpeg do you have installed? Do you have more than one version of ffmpeg installed?
Flags: needinfo?(r.rudolph)
Flags: needinfo?(mardeg)
ffmpeg -version
says 2.0
I can't tell if more than one version is installed.
other info:
libavcodec 55.18.102
libavformat 55.12.100
Flags: needinfo?(mardeg)
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.
(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.
Flags: needinfo?(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
Flags: needinfo?(feroz)
can reproduce with this last sample
Assignee: nobody → jyavenard
Priority: -- → P1
OS: Linux → All
Hardware: x86 → Unspecified
Keywords: regression
Summary: webm videos are not working any more on i686 Linux when media.format-reader.webm is set to true → webm videos are not working any more with new WebMDemuxer
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/
Attachment #8741623 - Flags: review?(matt.woodrow)
Attachment #8741624 - Flags: review?(matt.woodrow)
Attachment #8741625 - Flags: review?(matt.woodrow)
Attachment #8741626 - Flags: review?(matt.woodrow)
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
Flags: needinfo?(giles)
(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.
Blocks: 1154162
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/
Attachment #8741623 - Attachment description: MozReview Request: Bug 1243538: P1. Add VideoInfo.mOriginalImage member. r?mattwoodrow → MozReview Request: Bug 1243538: P1. Make MediaInfo::mImage an nsIntSize again and introduce a mImageRect member. r?mattwoodrow
Attachment #8741624 - Attachment description: MozReview Request: Bug 1243538: P2. Fix libvpx decoder when decoded video do not match container metadata. r?mattwoodrow → MozReview Request: Bug 1243538: P2. Add convenience VideoInfo::ScaledImageRect. r?mattwoodrow
Attachment #8741625 - Attachment description: MozReview Request: Bug 1243538: P3. Amend MP4Demuxer to initialize VideoInfo.mOriginalImage. r?mattwoodrow → MozReview Request: Bug 1243538: P3. Adjust libvpx decoder to allow different decoding size from metadata. r?mattwoodrow
Attachment #8741626 - Attachment description: MozReview Request: Bug 1243538: P4. Fix ffvpx decoder when decoded video do not match container metadata. r?mattwoodrow → MozReview Request: Bug 1243538: P4. Adjust ffvpx decoder to allow different decoding size from metadata. r?mattwoodrow
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 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 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 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 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 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/
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/
Attachment #8741793 - Flags: review?(cpearce)
What was the problem that caused the video to stop playing?
Flags: needinfo?(jyavenard)
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?
Attachment #8741793 - Flags: review?(cpearce)
(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.
Flags: needinfo?(jyavenard)
Flags: needinfo?(giles)
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 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
Attachment #8741793 - Flags: review?(cpearce)
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?
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.
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/
Attachment #8741793 - Attachment description: MozReview Request: Bug 1243538: P5. Adjust wmf decoder to allow different decoding size from metadata. r?cpearce → MozReview Request: Bug 1243538: P5. Adjust gonk decoder to allow different decoding size from metadata. r?alfredo
Attachment #8742163 - Flags: review?(bvandyk)
Attachment #8741793 - Flags: review?(cpearce) → review?(ayang)
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 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 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 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 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/
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/
Attachment #8741793 - Attachment description: MozReview Request: Bug 1243538: P5. Adjust gonk decoder to allow different decoding size from metadata. r?alfredo → MozReview Request: Bug 1243538: P6. Adjust gonk decoder to allow different decoding size from metadata. r?alfredo
Attachment #8742163 - Attachment description: MozReview Request: Bug 1243538: P6. Let the decoder handle picture resizing. r?SingingTree → MozReview Request: Bug 1243538: [webm] P7. Let the decoder handle picture resizing. r?SingingTree
Attachment #8742164 - Flags: review?(cpearce)
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 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/
sorry for the noise, I moved fix for bug 1154162 here, as really this belong as a block
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
Attachment #8741793 - Flags: review?(ayang) → review+
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.
Attachment #8742164 - Flags: review?(cpearce) → review+
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 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).
Attachment #8742164 - Flags: review+
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).
Attachment #8742163 - Flags: review?(bvandyk) → review+
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 :)
(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!
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 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 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 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 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 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 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 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/
jya: does this affect YouTube? If so we'll need to uplift this to 46.
Flags: needinfo?(jyavenard)
We may "wontfix" this, but it depends on what jya says about the impact of this bug on YouTube.
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).
Flags: needinfo?(jyavenard)
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
Attachment #8741623 - Flags: review?(matt.woodrow) → review+
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.
Attachment #8741624 - Flags: review?(matt.woodrow) → review+
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
Attachment #8741625 - Flags: review?(matt.woodrow) → review+
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
Attachment #8741626 - Flags: review?(matt.woodrow) → review+
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 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 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 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 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 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 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/
Depends on: 1266013
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!
Flags: needinfo?(mreavy)
Flags: needinfo?(jyavenard)
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...
Flags: needinfo?(jyavenard)
(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.)
Flags: needinfo?(mreavy)
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!
Flags: needinfo?(ajones)
I'd rather uplift these patches. They aren't especially complicated.
Flags: needinfo?(ajones)
Thanks Anthony. Jean-Yves, could you please nominate relevant patches for uplift to Beta47?
Flags: needinfo?(jyavenard)
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
Flags: needinfo?(jyavenard)
Attachment #8741623 - Flags: approval-mozilla-beta?
Attachment #8741623 - Flags: approval-mozilla-aurora?
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+
Attachment #8741623 - Flags: approval-mozilla-beta?
Attachment #8741623 - Flags: approval-mozilla-beta+
Attachment #8741623 - Flags: approval-mozilla-aurora?
Attachment #8741623 - Flags: approval-mozilla-aurora+
Hello Feroz, Mardeg, could you please verify this issue is fixed as expected on a latest Nightly build? Thanks!
Flags: needinfo?(mardeg)
Flags: needinfo?(feroz)
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.
Flags: needinfo?(jyavenard)
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
Flags: needinfo?(r.rudolph)
Flags: needinfo?(mardeg)
Flags: needinfo?(jyavenard)
Flags: needinfo?(feroz)
Flags: needinfo?(jyavenard)
(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!
(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.
Status: RESOLVED → VERIFIED
(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).
Flags: needinfo?(jyavenard)
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
QA Whiteboard: [good first verify]
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.
Reproduced this issue using Firefox 48.0 b3  on windows 7 x64 .
Confirming the fix on Firefox 48.0 b3
You need to log in before you can comment on or make changes to this bug.