VPXDecoder does not decode alpha frames

RESOLVED FIXED in Firefox 53

Status

()

Core
Audio/Video: Playback
RESOLVED FIXED
5 months ago
a month ago

People

(Reporter: Karolien, Assigned: Karolien)

Tracking

(Blocks: 1 bug)

unspecified
mozilla53
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox53 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(5 attachments, 1 obsolete attachment)

(Assignee)

Description

5 months ago
See bug 944117 for overview. As part of implementing the WebM alpha the VPXDecoder needs to be updated. Using the VPXDecoder gives us the best coverage in terms of platforms. The PDMFactory will need to fall back to using the VPXDecoder if alpha is present. Some data structures may need to be updated to accommodate alpha.
(Assignee)

Updated

5 months ago
Blocks: 944117
Comment hidden (mozreview-request)

Comment 2

5 months ago
mozreview-review
Comment on attachment 8815450 [details]
Bug 1321076 - Updated VPXDecoder to decode alpha frames.

https://reviewboard.mozilla.org/r/96350/#review96600

::: dom/media/platforms/agnostic/VPXDecoder.h:72
(Diff revision 1)
>  
>    const VideoInfo& mInfo;
>  
>    const int mCodec;
> +
> +  bool mHasAlphaContext;

We could init this C++11 style as with the flag from the previous changeset. I.e. bool foo = false here rather than in the init list.

::: dom/media/platforms/ffmpeg/FFmpegDecoderModule.h:37
(Diff revision 1)
>  
>    already_AddRefed<MediaDataDecoder>
>    CreateVideoDecoder(const CreateDecoderParams& aParams) override
>    {
> +    // Temporary - forces use of VPXDecoder when alpha is present.
> +    // Bug 1263836 will handle alpha scenario once implemented. 

Can extend the comment to refect that 1263836 will enable us to shift this check into the PDMfactory, but won't of itself remove the need for a check.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

5 months ago
Assignee: nobody → kkoorts
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

4 months ago
Attachment #8815450 - Flags: feedback?(bvandyk)
(Assignee)

Updated

4 months ago
Attachment #8819140 - Flags: feedback?(bvandyk)
(Assignee)

Updated

4 months ago
Attachment #8819141 - Flags: feedback?(bvandyk)

Comment 10

4 months ago
mozreview-review
Comment on attachment 8819140 [details]
Bug 1321076 - PDMFactory defaults to using VPXDecoder when alpha is present.

https://reviewboard.mozilla.org/r/98972/#review99600

Looks good to me. Couple of comments for your consideration and a whitespace nit. Since this is a feedback I'm gonna leave the flag as is.

::: dom/media/MediaData.cpp:317
(Diff revision 2)
>                                      aTimecode,
>                                      aInfo.mDisplay,
>                                      0));
> +
> +  // If alpha is present, convert from YUVA to BGRA format on the software side.
> +  if(aBuffer.mPlanes[3].mData){

Not saying we should change this right now, but in general think we should be mindful of the differing levels of abstractions that we create images.

::: gfx/ycbcr/yuv_convert.h:120
(Diff revision 2)
> +                           int uvstride,
> +                           int bgrastride);
> +
>  } // namespace gfx
>  } // namespace mozilla
>   

Trailing whitespace

::: gfx/ycbcr/yuv_convert.cpp:521
(Diff revision 2)
> +                           int uv_pitch,
> +                           int bgra_pitch) {
> +
> +  // Despite the name of this method, the buffer that comes out is in fact in
> +  // BGRA format. In memory it is inverted.
> +  // Attenuate is used when converting to BGRA.

Could be more verbose and say the downstream graphics stack expects an attenunated input, which is why we do it here.
Attachment #8815450 - Flags: feedback?(bvandyk)
Attachment #8819140 - Flags: feedback?(bvandyk)
Attachment #8819141 - Flags: feedback?(bvandyk)

Comment 11

4 months ago
mozreview-review-reply
Comment on attachment 8819140 [details]
Bug 1321076 - PDMFactory defaults to using VPXDecoder when alpha is present.

https://reviewboard.mozilla.org/r/98972/#review99600

> Not saying we should change this right now, but in general think we should be mindful of the differing levels of abstractions that we create images.

Actually, just ignore my comment for now, we can look at how the colour conversion and image creation should fit in once we've got this landed if we need to.
(Assignee)

Comment 12

4 months ago
mozreview-review
Comment on attachment 8819140 [details]
Bug 1321076 - PDMFactory defaults to using VPXDecoder when alpha is present.

https://reviewboard.mozilla.org/r/98972/#review99612
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 19

4 months ago
mozreview-review
Comment on attachment 8815450 [details]
Bug 1321076 - Updated VPXDecoder to decode alpha frames.

https://reviewboard.mozilla.org/r/96350/#review99628

This will cause regressions with existing player (see bug 1194884)

::: dom/media/platforms/agnostic/VPXDecoder.cpp:96
(Diff revision 5)
>  }
>  
>  RefPtr<MediaDataDecoder::InitPromise>
>  VPXDecoder::Init()
>  {
> -  int decode_threads = 2;
> +  if (NS_FAILED(InitContext(&mVPX, mInfo, mCodec))) {

not sure how this helps to move the code to a new static InitContext.

there's no change to the code there

::: dom/media/platforms/agnostic/VPXDecoder.cpp:143
(Diff revision 5)
>    }
>  
>    vpx_codec_iter_t  iter = nullptr;
>    vpx_image_t      *img;
>  
> -  while ((img = vpx_codec_get_frame(&mVPX, &iter))) {
> +  img = vpx_codec_get_frame(&mVPX, &iter);

This will regress bug 1194884.

You must loop to retrieve all frames potentially found in a packet.

See linked bug for details.

We typically don't have code that serve no purpose, so removing it is often not a good idea. git/hg blame is a useful tool to determine on why some code was added.

::: dom/media/platforms/agnostic/VPXDecoder.cpp:148
(Diff revision 5)
> -  while ((img = vpx_codec_get_frame(&mVPX, &iter))) {
> +  img = vpx_codec_get_frame(&mVPX, &iter);
> -    NS_ASSERTION(img->fmt == VPX_IMG_FMT_I420 ||
> +  NS_ASSERTION(img->fmt == VPX_IMG_FMT_I420 ||
> -                 img->fmt == VPX_IMG_FMT_I444,
> +                img->fmt == VPX_IMG_FMT_I444,
> -                 "WebM image format not I420 or I444");
> +                "WebM image format not I420 or I444");
>  
> +  vpx_image_t *img_alpha = nullptr;

I'd much prefer that this is only called if there's no alpha plane.
So that it's not up to DecodeAlpha to test that the values provided are valid.
Attachment #8815450 - Flags: review?(jyavenard) → review-

Comment 20

4 months ago
mozreview-review
Comment on attachment 8819141 [details]
Refactored YCbCrBuffer to YCbCrABuffer.

https://reviewboard.mozilla.org/r/98974/#review99634

Should have a separate YCbCrABuffer , that inherit from YCbCrBuffer so you don't have to modify all codes using and keep changes narrowed.
E.g. only use a YCbCrABuffer when we know we have an alpha plane. 

And in this case, it would serve no purpose if you perform the RGB conversion directly inside the VPXDecoder.

That is, if no alpha, return a VideoData using a plain YCbCrBuffer as backend. if alpha, then return a VideoData using a RGBA image instead
Attachment #8819141 - Flags: review?(jyavenard) → review-
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 23

4 months ago
mozreview-review
Comment on attachment 8819140 [details]
Bug 1321076 - PDMFactory defaults to using VPXDecoder when alpha is present.

https://reviewboard.mozilla.org/r/98972/#review99636

::: dom/media/MediaData.cpp:316
(Diff revision 5)
>                                      aKeyframe,
>                                      aTimecode,
>                                      aInfo.mDisplay,
>                                      0));
> +
> +  // If alpha is present, convert from YUVA to BGRA format on the software

It would be much easier (and avoid having to refactor the whole YUV buffer class) by keeping the existing code path if non alpha

And if alpha, then directly decode into RGBA from the two YUV buffers (with 3 planes in use for the main one, and only the first plane used for the alpha layer)

No need to have an intermediary YUVA which is almost never used

::: dom/media/MediaData.cpp:332
(Diff revision 5)
> +                                      aBuffer.mPlanes[0].mHeight),
> +                              SurfaceFormat::B8G8R8A8)) {
> +      return nullptr;
> +    }
> +    uint8_t* argb_buffer = videoImage->GetBuffer();
> +    auto size = videoImage->GetSize();

don't use auto here, it doesn't simplify much, and make the code harder to review

::: dom/media/MediaData.cpp:336
(Diff revision 5)
> +    uint8_t* argb_buffer = videoImage->GetBuffer();
> +    auto size = videoImage->GetSize();
> +
> +    // Despite the name of this method, the buffer that comes out is in fact in
> +    // BGRA format. In memory it is inverted.
> +    ConvertYCbCrAToARGB(aBuffer.mPlanes[0].mData,

shouldn't we rename the function instead?
Attachment #8819140 - Flags: review?(jyavenard) → review-
(In reply to Jean-Yves Avenard [:jya] from comment #23)
> It would be much easier (and avoid having to refactor the whole YUV buffer
> class) by keeping the existing code path if non alpha

I'd argue that extending VideoData::CreateAndCopyData() to handle alpha is consistent with its purpose. It is a smaller diff to only modify VPXDecoder but that doesn't make it architecturally better.
(In reply to Anthony Jones (:kentuckyfriedtakahe, :k17e) [afk 24 Dec - 8 Jan] from comment #24)
> (In reply to Jean-Yves Avenard [:jya] from comment #23)
> > It would be much easier (and avoid having to refactor the whole YUV buffer
> > class) by keeping the existing code path if non alpha
> 
> I'd argue that extending VideoData::CreateAndCopyData() to handle alpha is
> consistent with its purpose. It is a smaller diff to only modify VPXDecoder
> but that doesn't make it architecturally better.

There's no reason why we can't have a specialised CreateAndCopy that specifically handles YUVABuffer.

It's the effective removal of YUVBuffer I'm most bothered about for little benefit. And we already have a different RGB Image different to RGBA image too.

Modifying everything for what is otherwise a very little used occurrence has the potential to introduce unexpected regressions.

Comment 26

4 months ago
mozreview-review
Comment on attachment 8815450 [details]
Bug 1321076 - Updated VPXDecoder to decode alpha frames.

https://reviewboard.mozilla.org/r/96350/#review100030

::: dom/media/platforms/agnostic/VPXDecoder.cpp:267
(Diff revision 5)
> +        NS_ERROR_DOM_MEDIA_DECODE_ERR,
> +        RESULT_DETAIL("VPX failed to initialize alpha context"));
> +    }
> +    mHasAlphaContext = true;
> +  }
> +  if (vpx_codec_err_t r = vpx_codec_decode(&mVPXAlpha,

initialise r prior the test
(Assignee)

Comment 27

4 months ago
mozreview-review-reply
Comment on attachment 8815450 [details]
Bug 1321076 - Updated VPXDecoder to decode alpha frames.

https://reviewboard.mozilla.org/r/96350/#review99628

> not sure how this helps to move the code to a new static InitContext.
> 
> there's no change to the code there

The idea behind moving the logic of InitContext into a static method was to accommodate the second decoder, and avoid code duplication.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

4 months ago
Attachment #8819141 - Attachment is obsolete: true
Attachment #8819141 - Flags: review?(jyavenard)
Comment hidden (mozreview-request)

Comment 32

4 months ago
mozreview-review
Comment on attachment 8815450 [details]
Bug 1321076 - Updated VPXDecoder to decode alpha frames.

https://reviewboard.mozilla.org/r/96350/#review100398

r=me with comment addressed.

::: dom/media/platforms/agnostic/VPXDecoder.cpp:260
(Diff revision 6)
> +MediaResult
> +VPXDecoder::DecodeAlpha(vpx_image_t** aImgAlpha,
> +                        MediaRawData* aSample)
> +{
> +  if (!mHasAlphaContext) {
> +    if (NS_FAILED(InitContext(&mVPXAlpha, mInfo, mCodec))) {

has discussed previously, please move initialisation in Init method (which is to be performed only if we have an alpha plane)

you then don't need mHasAlphaContext member.
Attachment #8815450 - Flags: review?(jyavenard) → review+

Comment 33

4 months ago
mozreview-review
Comment on attachment 8819140 [details]
Bug 1321076 - PDMFactory defaults to using VPXDecoder when alpha is present.

https://reviewboard.mozilla.org/r/98972/#review100402

::: dom/media/platforms/android/AndroidDecoderModule.cpp:173
(Diff revision 7)
>  }
>  
>  already_AddRefed<MediaDataDecoder>
>  AndroidDecoderModule::CreateVideoDecoder(const CreateDecoderParams& aParams)
>  {
> +  // Temporary - forces use of VPXDecoder when alpha is present.

this seems out of scope to me. please move this to another separate patch.

::: gfx/ycbcr/YCbCrUtils.cpp:156
(Diff revision 7)
>                            yuvtype,
>                            aData.mYUVColorSpace);
>    }
>  }
>  
> +void

this function seems a tad wasteful to me.

do you really need to create a wrapping function only to change the order of 2 arguments?
Attachment #8819140 - Flags: review?(jyavenard) → review+

Comment 34

4 months ago
mozreview-review
Comment on attachment 8820425 [details]
Bug 1321076 - New overloaded CreateAndCopy method that takes alpha plane and returns VideoData with SharedRGBImage, in the case of alpha.

https://reviewboard.mozilla.org/r/99910/#review100404

Much better !!! Isn't it much more elegant that way ?

It would be better to split this patch in two. One adding the method CreateAndCopyData for YUVA, and one finally using it in VPXDecoder.

Still think you could do without most of P2 and directly call ConvertYCbCrAToARGB with the right argument order

::: dom/media/platforms/agnostic/VPXDecoder.cpp:207
(Diff revision 2)
> -                                   b,
> +                                       b,
> -                                   aSample->mKeyframe,
> +                                       aSample->mKeyframe,
> -                                   aSample->mTimecode,
> +                                       aSample->mTimecode,
> -                                   mInfo.ScaledImageRect(img->d_w,
> +                                       mInfo.ScaledImageRect(img->d_w,
> -                                                         img->d_h));
> +                                                             img->d_h));
> +    }

} else {
Attachment #8820425 - Flags: review?(jyavenard) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

4 months ago
Keywords: checkin-needed
Please post try runs...
(Assignee)

Comment 41

4 months ago
I've done a try with my changes and a central; the issues with telemetry appear to be shared.

With changes: 
https://treeherder.mozilla.org/#/jobs?repo=try&revision=db88cb97951e994d1d0feac97dd394d6f8543068

Central without changes:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=633fbf3a46c2347d50aa9de8c96dbd3b2250805c

Comment 42

4 months ago
mozreview-review
Comment on attachment 8820843 [details]
Bug 1321076 - In the case of alpha, VPXDecoder uses overloaded CreateAndCopy that takes alpha plane.

https://reviewboard.mozilla.org/r/100246/#review100840

::: dom/media/platforms/agnostic/VPXDecoder.cpp:202
(Diff revision 1)
>        return MediaResult(NS_ERROR_DOM_MEDIA_DECODE_ERR,
>                           RESULT_DETAIL("VPX Unknown image format"));
>      }
>  
> -    RefPtr<VideoData> v =
> -      VideoData::CreateAndCopyData(mInfo,
> +    RefPtr<VideoData> v;
> +    if(!img_alpha){

space missing between if and (

::: dom/media/platforms/agnostic/VPXDecoder.cpp:214
(Diff revision 1)
> -                                   aSample->mKeyframe,
> +                                       aSample->mKeyframe,
> -                                   aSample->mTimecode,
> +                                       aSample->mTimecode,
> -                                   mInfo.ScaledImageRect(img->d_w,
> +                                       mInfo.ScaledImageRect(img->d_w,
> -                                                         img->d_h));
> +                                                             img->d_h));
> +    }
> +    else {

} else {
Attachment #8820843 - Flags: review?(jyavenard) → review+

Comment 43

4 months ago
mozreview-review
Comment on attachment 8820842 [details]
Bug 1321076 - Added util functions to help with YUVA to BGRA conversion.

https://reviewboard.mozilla.org/r/100244/#review100842
Attachment #8820842 - Flags: review?(jyavenard) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 49

4 months ago
mozreview-review
Comment on attachment 8820843 [details]
Bug 1321076 - In the case of alpha, VPXDecoder uses overloaded CreateAndCopy that takes alpha plane.

https://reviewboard.mozilla.org/r/100246/#review100846

Comment 50

4 months ago
Pushed by ihsiao@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7d007364e2cf
Updated VPXDecoder to decode alpha frames. r=jya
https://hg.mozilla.org/integration/autoland/rev/ffe69d7de0a8
PDMFactory defaults to using VPXDecoder when alpha is present. r=jya
https://hg.mozilla.org/integration/autoland/rev/ed324bc29fef
Added util functions to help with YUVA to BGRA conversion. r=jya
https://hg.mozilla.org/integration/autoland/rev/34d22d965f68
New overloaded CreateAndCopy method that takes alpha plane and returns VideoData with SharedRGBImage, in the case of alpha. r=jya
https://hg.mozilla.org/integration/autoland/rev/ce67239948a0
In the case of alpha, VPXDecoder uses overloaded CreateAndCopy that takes alpha plane. r=jya
Keywords: checkin-needed

Comment 51

4 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/7d007364e2cf
https://hg.mozilla.org/mozilla-central/rev/ffe69d7de0a8
https://hg.mozilla.org/mozilla-central/rev/ed324bc29fef
https://hg.mozilla.org/mozilla-central/rev/34d22d965f68
https://hg.mozilla.org/mozilla-central/rev/ce67239948a0
Status: NEW → RESOLVED
Last Resolved: 4 months ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Depends on: 1325707
No longer depends on: 1325707

Updated

3 months ago
Depends on: 1329104
Blocks: 1348347
You need to log in before you can comment on or make changes to this bug.