Closed Bug 1321076 Opened 8 years ago Closed 8 years ago

VPXDecoder does not decode alpha frames

Categories

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

Product:
Core
Component:
Audio/Video: Playback
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla53
Tracking Flags:
Tracking Status
firefox53 --- fixed

People

(Reporter: kkoorts, Assigned: kkoorts)

References

(Blocks 1 open bug)

Details

Attachments

(5 files, 1 obsolete file)

Bug 1321076 - Updated VPXDecoder to decode alpha frames.
58 bytes, text/x-review-board-request
jya
: review+
Details
Bug 1321076 - PDMFactory defaults to using VPXDecoder when alpha is present.
58 bytes, text/x-review-board-request
jya
: review+
Details
Bug 1321076 - New overloaded CreateAndCopy method that takes alpha plane and returns VideoData with SharedRGBImage, in the case of alpha.
58 bytes, text/x-review-board-request
jya
: review+
Details
Bug 1321076 - Added util functions to help with YUVA to BGRA conversion.
59 bytes, text/x-review-board-request
jya
: review+
Details
Bug 1321076 - In the case of alpha, VPXDecoder uses overloaded CreateAndCopy that takes alpha plane.
59 bytes, text/x-review-board-request
jya
: review+
Details
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.
Blocks: 944117
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.
Assignee: nobody → kkoorts
Attachment #8815450 - Flags: feedback?(bvandyk)
Attachment #8819140 - Flags: feedback?(bvandyk)
Attachment #8819141 - Flags: feedback?(bvandyk)
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.
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.
Comment on attachment 8819140 [details] Bug 1321076 - PDMFactory defaults to using VPXDecoder when alpha is present. https://reviewboard.mozilla.org/r/98972/#review99612
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 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 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 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
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.
Attachment #8819141 - Attachment is obsolete: true
Attachment #8819141 - Flags: review?(jyavenard)
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 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 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+
Keywords: checkin-needed
Please post try runs...
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 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 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 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
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
Depends on: 1325707
No longer depends on: 1325707
Depends on: 1329104
See Also: → 1366502
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: