Closed Bug 1484783 Opened 6 years ago Closed 6 years ago

SW decode of certain video sizes causes corruption

Categories

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

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

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla64
Tracking Flags:
Tracking Status
firefox-esr60 63+ verified
firefox62 --- wontfix
firefox63 + verified
firefox64 + verified

People

(Reporter: paul.blinzer, Assigned: jya)

Details

(Keywords: regression)

Attachments

(1 file)

Bug 1484783 - Ensure to read the default stride when hardware acceleration isn't usable. r?mattwoodrow
46 bytes, text/x-phabricator-request
mattwoodrow
: review+
pascalc
: approval-mozilla-beta+
RyanVM
: approval-mozilla-esr60+
Details | Review 
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:61.0) Gecko/20100101 Firefox/61.0
Build ID: 20180807170231

Steps to reproduce:

1.  Install the latest version Firefox browser on a system with AMD GPU (but repro has been seen on other systems). 
2.  Switch browser to SW decode 
3.  Open the following websites and select the playback resolution (if possible) or vary the size of the window. 
        cnn
        cp24
        youtube



Actual results:

Depending on the size of the window,corruption is seen. 

SW decode uses YV12 raw data and three surfaces are created (named as A, B & C) and will receive YUV data. Y is placed in A, V to B and U to C. 

When video size is 640x360, the ResourceUpdateSubresourceUP() API call sees the following parameters:

ResourceUpdateSubresourceUP(A,0,pDstBox,pRawData0,640,0,*)；
ResourceUpdateSubresourceUP(B,0,pDstBox,pRawData1,320,0,*)；
ResourceUpdateSubresourceUP(C,0,pDstBox,pRawData2,320,0,*)；

When video size is 400x224, the driver's ResourceUpdateSubresourceUP() call sees the following call parameters:

ResourceUpdateSubresourceUP(A,0,pDstBox,pRawData0,0,0,*)；
ResourceUpdateSubresourceUP(B,0,pDstBox,pRawData0,0,0,*)；
ResourceUpdateSubresourceUP(C,0,pDstBox,pRawData0,0,0,*)；

The parameters are as follows. 

ResourceUpdateSubresourceUP(
    DxResource*          pDstResource,           ///< destination resource
    UINT                 dstSubresourceIndex,    ///< destination subresource index
    const D3D10_DDI_BOX* pDstBox,                ///< destination box
    const VOID*          pSrcDataUP,             ///< pointer to source data
    UINT                 srcPitch,               ///< source data pitch
    UINT                 srcSlicePitch,          ///< source data slicePitch
    const DxCopyFlags*   pCopyFlags)             ///< update subresource flags

In other words for certain video resolutions (e.g. 400x224) the pitch parameter is forced to 0 by the application layer, which seems the root of this issue.


Expected results:

SW decode & playback through browser works without corruption on all resolutions.
Correction for step 2: Switch driver into SW decode e.g. don't expose the hardware decoder in the driver, forcing to use the SW decoder path in the browser.
Component: Untriaged → Audio/Video: Playback
Product: Firefox → Core
Bas, you might be interested in this.
Flags: needinfo?(bas)
Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: -- → P2
It seems the root cause may be located in this code, if the twoDBuffer->Lock2D function call fails for some reason. 

HRESULT
WMFVideoMFTManager::CreateBasicVideoFrame(IMFSample* aSample,
                                          int64_t aStreamOffset,
                                          VideoData** aOutVideoData)
{
  NS_ENSURE_TRUE(aSample, E_POINTER);
  NS_ENSURE_TRUE(aOutVideoData, E_POINTER);

  *aOutVideoData = nullptr;

  HRESULT hr;
  RefPtr<IMFMediaBuffer> buffer;

  // Must convert to contiguous buffer to use IMD2DBuffer interface.
  hr = aSample->ConvertToContiguousBuffer(getter_AddRefs(buffer));
  NS_ENSURE_TRUE(SUCCEEDED(hr), hr);

  // Try and use the IMF2DBuffer interface if available, otherwise fallback
  // to the IMFMediaBuffer interface. Apparently IMF2DBuffer is more efficient,
  // but only some systems (Windows 8?) support it.
  BYTE* data = nullptr;
  LONG stride = 0;
  RefPtr<IMF2DBuffer> twoDBuffer;
  hr = buffer->QueryInterface(
    static_cast<IMF2DBuffer**>(getter_AddRefs(twoDBuffer)));
  if (SUCCEEDED(hr)) {
    hr = twoDBuffer->Lock2D(&data, &stride);
    NS_ENSURE_TRUE(SUCCEEDED(hr), hr);
  } else {
    hr = buffer->Lock(&data, nullptr, nullptr);
    NS_ENSURE_TRUE(SUCCEEDED(hr), hr);
    stride = mVideoStride;
  }

  // YV12, planar format: [YYYY....][VVVV....][UUUU....]
  // i.e., Y, then V, then U.
  VideoData::YCbCrBuffer b;

  uint32_t videoWidth = mImageSize.width;
  uint32_t videoHeight = mImageSize.height;

  // Y (Y') plane
  b.mPlanes[0].mData = data;
  b.mPlanes[0].mStride = stride;
  b.mPlanes[0].mHeight = videoHeight;
  b.mPlanes[0].mWidth = videoWidth;
  b.mPlanes[0].mOffset = 0;
  b.mPlanes[0].mSkip = 0;

  MOZ_DIAGNOSTIC_ASSERT(mDecodedImageSize.height % 16 == 0,
                        "decoded height must be 16 bytes aligned");
  uint32_t y_size = stride * mDecodedImageSize.height;
  uint32_t v_size = stride * mDecodedImageSize.height / 4;
  uint32_t halfStride = (stride + 1) / 2;
  uint32_t halfHeight = (videoHeight + 1) / 2;
  uint32_t halfWidth = (videoWidth + 1) / 2;

  // U plane (Cb)
  b.mPlanes[1].mData = data + y_size + v_size;
  b.mPlanes[1].mStride = halfStride;
  b.mPlanes[1].mHeight = halfHeight;
  b.mPlanes[1].mWidth = halfWidth;
  b.mPlanes[1].mOffset = 0;
  b.mPlanes[1].mSkip = 0;

  // V plane (Cr)
  b.mPlanes[2].mData = data + y_size;
  b.mPlanes[2].mStride = halfStride;
  b.mPlanes[2].mHeight = halfHeight;
  b.mPlanes[2].mWidth = halfWidth;
  b.mPlanes[2].mOffset = 0;
  b.mPlanes[2].mSkip = 0;

  // YuvColorSpace
  b.mYUVColorSpace = mYUVColorSpace;

  TimeUnit pts = GetSampleTime(aSample);
  NS_ENSURE_TRUE(pts.IsValid(), E_FAIL);
  TimeUnit duration = GetSampleDuration(aSample);
  NS_ENSURE_TRUE(duration.IsValid(), E_FAIL);
  gfx::IntRect pictureRegion =
    mVideoInfo.ScaledImageRect(videoWidth, videoHeight);

  if (!mKnowsCompositor || !mKnowsCompositor->SupportsD3D11() || !mIMFUsable) {
    RefPtr<VideoData> v =
      VideoData::CreateAndCopyData(mVideoInfo,
                                   mImageContainer,
                                  aStreamOffset,
                                   pts,
                                   duration,
                                   b,
                                   false,
                                   TimeUnit::FromMicroseconds(-1),
                                   pictureRegion);
    if (twoDBuffer) {
      twoDBuffer->Unlock2D();
    } else {
      buffer->Unlock();
    }
    v.forget(aOutVideoData);
    return S_OK;
  }

  RefPtr<layers::PlanarYCbCrImage> image =
    new IMFYCbCrImage(buffer, twoDBuffer, mKnowsCompositor, mImageContainer);

  VideoData::SetVideoDataToImage(image,
                                 mVideoInfo,
                                 b,
                                 pictureRegion,
                                 false);

  RefPtr<VideoData> v =
    VideoData::CreateFromImage(mVideoInfo.mDisplay,
                               aStreamOffset,
                               pts,
                               duration,
                               image.forget(),
                               false,
                               TimeUnit::FromMicroseconds(-1));

  v.forget(aOutVideoData);
  return S_OK;
}
On some platforms where a hardware decoder is present, but non functioning, we would fail to initialize the video stride, leading to the frames being incorrectly displayed later.

Also delete the DXVA2 manager early under those circumstances
Comment on attachment 9007714 [details]
Bug 1484783 - Ensure to read the default stride when hardware acceleration isn't usable. r?mattwoodrow

Matt Woodrow (:mattwoodrow) has approved the revision.
Attachment #9007714 - Flags: review+
Pushed by jyavenard@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b5fd2f7a22d9
Ensure to read the default stride when hardware acceleration isn't usable. r=mattwoodrow
https://hg.mozilla.org/mozilla-central/rev/b5fd2f7a22d9
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox64: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
[Tracking Requested - why for this release]: will affect all systems where HW decoding is present but we determined that it wasn't usable
Assignee: nobody → jyavenard
status-firefox62: --- → wontfix
status-firefox63: --- → affected
tracking-firefox63: --- → ?
Flags: needinfo?(bas)
Could you please check that the issue is resolved for you in the next nightly?

thank you
Flags: needinfo?(paul.blinzer)
Comment on attachment 9007714 [details]
Bug 1484783 - Ensure to read the default stride when hardware acceleration isn't usable. r?mattwoodrow

Approval Request Comment
[Feature/Bug causing the regression]: bug 1193547 
[User impact if declined]: Invalid decoding when using software decoder on machines with blacklisted HW decoder
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: np
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: we properly initialize the image stride. Otherwise it would be set with a value of 0 causing images to be incorrectly displayed.
[String changes made/needed]: none

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: videos not being displayed correctly.
Fix Landed on Version: 64
Risk to taking this patch (and alternatives if risky): no
String or UUID changes made by this patch: none

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.
Attachment #9007714 - Flags: approval-mozilla-esr60?
Attachment #9007714 - Flags: approval-mozilla-beta?
We have tested the nightly on Windows10 and Windows 7 and confirmed the issue is fully fixed now.
Flags: needinfo?(paul.blinzer)
Comment on attachment 9007714 [details]
Bug 1484783 - Ensure to read the default stride when hardware acceleration isn't usable. r?mattwoodrow

Fix confirmed on Nightly by QA, uplift approved for 63 beta 8, thanks.
Attachment #9007714 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment on attachment 9007714 [details]
Bug 1484783 - Ensure to read the default stride when hardware acceleration isn't usable. r?mattwoodrow

Fixes invalid decoding when using software decoder on machines with blacklisted HW decoder. Approved for ESR 60.3.
Attachment #9007714 - Flags: approval-mozilla-esr60? → approval-mozilla-esr60+
tracking-firefox-esr60: ? → 63+
Flags: qe-verify+
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.13; rv:63.0) Gecko/20100101 Firefox/63.0
Build ID: 20181008155858

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.13; rv:60.0) Gecko/20100101 Firefox/60.0
Build ID: 20181006124451

Verified as fixed on the latest Beta build (v63beta13) and also on the latest ESR build (60.2.3).
Status: RESOLVED → VERIFIED
status-firefox63: fixed → verified
status-firefox64: fixed → verified
status-firefox-esr60: fixed → verified
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: