Skip to content
This repository has been archived by the owner on May 17, 2023. It is now read-only.

[vpp] Enable copy pass thru for VAAPI #1268

Merged
merged 1 commit into from Mar 21, 2019
Merged

[vpp] Enable copy pass thru for VAAPI #1268

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 2 additions & 4 deletions _studio/mfx_lib/shared/src/mfx_session.cpp
View file
Open in desktop
@@ -1,4 +1,4 @@
// Copyright (c) 2017 Intel Corporation
// Copyright (c) 2017-2019 Intel Corporation
//
// Permission is hereby granted, free of charge, to any person obtaining a copy
// of this software and associated documentation files (the "Software"), to deal
Expand Down Expand Up @@ -920,9 +920,7 @@ mfxStatus _mfxSession_1_10::InitEx(mfxInitParam& par)
if (pCmCore)
{
mfxRes = pCmCore->SetCmCopyStatus(false);
}
if (MFX_ERR_NONE != mfxRes) {
return mfxRes;
MFX_CHECK_STS(mfxRes);
}
}

Expand Down
2 changes: 2 additions & 0 deletions _studio/mfx_lib/vpp/include/mfx_vpp_hw.h
View file
Open in desktop
Expand Up @@ -908,6 +908,8 @@ namespace MfxHwVideoProcessing
mfxFrameSurface1 *pInputSurface,
mfxFrameSurface1 *pOutputSurface);

bool UseCopyPassThrough(const DdiTask *pTask) const;

mfxStatus PreWorkOutSurface(ExtSurface & output);
mfxStatus PreWorkInputSurface(std::vector<ExtSurface> & surfQueue);

Expand Down
33 changes: 27 additions & 6 deletions _studio/mfx_lib/vpp/src/mfx_vpp_hw.cpp
View file
Open in desktop
@@ -1,4 +1,4 @@
// Copyright (c) 2018 Intel Corporation
// Copyright (c) 2018-2019 Intel Corporation
//
// Permission is hereby granted, free of charge, to any person obtaining a copy
// of this software and associated documentation files (the "Software"), to deal
Expand Down Expand Up @@ -2962,6 +2962,28 @@ mfxStatus VideoVPPHW::Close()
} // mfxStatus VideoVPPHW::Close()


bool VideoVPPHW::UseCopyPassThrough(const DdiTask *pTask) const
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the name of the function should probably be: UseCmCopyPassThrough(), right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The plan was to hide mere presence of Cm. Why may a user of CopyPassThrought be interested in what is used inside? One just want fastest version, I think.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because it seems that this function implies that CM is going to be used. At least that's what I think looking into last line in its code.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I got it after offline discussion. Under copy pass through we understand any mem copy operation without VP algorithms. This can be map+memcpy or cmcopy. In different modes we prefer different copy type.

{
if (!m_config.m_bCopyPassThroughEnable
|| IsRoiDifferent(pTask->input.pSurf, pTask->output.pSurf))
{
return false;
}

if (m_pCore->GetVAType() != MFX_HW_VAAPI || m_ioMode != D3D_TO_D3D)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't this check mean that vaapi core is used?
I.e. cast on line 2979 wouldn't fail?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mgonchar Yes, the cast must not fail. I don't like dropping the check, however. What can I do here, as there is no asserts in release and you think returning mfxStatus is ugly?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well, currently I see two different ways:

  1. Leave the current code, but return false instead of error if cast fails (which is supposed to be almost impossible situation). So we just silently hide the error (not actually so, because we can use trace macro to at least signal to stdout if traces are on)
  2. Move to references casting, then some upper layers should deal with possible exception (which is supposed to be almost impossible situation). So we loose information about place of error occurrence.

So both of these ways are definitely not much better than current approach. I will try to figure out better solution...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Asserts are enabled in release builds of master branch. They are switched off in builds of release branches. So, use assert if you feel it right.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here I want to be protected from some fearless code refactoring. So checking on master' release is just fine.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having virtual GetCmCopyStatus in core is not an option to avoid this cast which is ugly on its own?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe adding new virtual functions to CommonCORE is prohibited. That's why the cast was applied here.

{
return true;
}

// Under VAAPI for D3D_TO_D3D either CM copy or VPP pipeline are preferred by
// performance reasons. So, before enabling CopyPassThrough for a task we check
// for CM availability to do fallback to VPP if there is no CM.
// This can't be done in ConfigureExecuteParams() because CmCopy status is set later.
const VAAPIVideoCORE *vaapiCore = dynamic_cast<VAAPIVideoCORE*>(m_pCore);
MFX_CHECK_WITH_ASSERT(vaapiCore, false);
return vaapiCore->CmCopy();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you know, I am not sure I like this code. I would probably go with just:

const VAAPIVideoCORE *vaapiCore = dynamic_cast<VAAPIVideoCORE*>(m_pCore);
return (vaapiCore)? vaapiCore->CmCopy(): false;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a huge "Can't happen!" and we are ignoring it silently, aren't we? Why not add
if(!vaapiCore) MFX_STS_TRACE(); at least?

}

mfxStatus VideoVPPHW::VppFrameCheck(
mfxFrameSurface1 *input,
mfxFrameSurface1 *output,
Expand Down Expand Up @@ -3115,9 +3137,11 @@ mfxStatus VideoVPPHW::VppFrameCheck(
#endif
MFX_CHECK_STS(sts);

bool useCopyPassThrough = UseCopyPassThrough(pTask);

if (VPP_SYNC_WORKLOAD == m_workloadMode)
{
pTask->bRunTimeCopyPassThrough = (true == m_config.m_bCopyPassThroughEnable && false == IsRoiDifferent(pTask->input.pSurf, pTask->output.pSurf));
pTask->bRunTimeCopyPassThrough = useCopyPassThrough;

// submit task
SyncTaskSubmission(pTask);
Expand All @@ -3136,8 +3160,7 @@ mfxStatus VideoVPPHW::VppFrameCheck(
}
else
{
if (false == m_config.m_bCopyPassThroughEnable ||
(true == m_config.m_bCopyPassThroughEnable && true == IsRoiDifferent(pTask->input.pSurf, pTask->output.pSurf)))
if (!useCopyPassThrough)
{
pTask->bRunTimeCopyPassThrough = false;

Expand Down Expand Up @@ -6104,7 +6127,6 @@ mfxStatus ConfigureExecuteParams(
executeParams.bSceneDetectionEnable = false;
}

#if defined(WIN64) || defined (WIN32)
if ( (0 == memcmp(&videoParam.vpp.In, &videoParam.vpp.Out, sizeof(mfxFrameInfo))) &&
executeParams.IsDoNothing() )
{
Expand All @@ -6115,7 +6137,6 @@ mfxStatus ConfigureExecuteParams(
config.m_bCopyPassThroughEnable = false;// after Reset() parameters may be changed,
// flag should be disabled
}
#endif//m_bCopyPassThroughEnable == false for another OS

if (inDNRatio == outDNRatio && !executeParams.bVarianceEnable && !executeParams.bComposite &&
!(config.m_extConfig.mode == IS_REFERENCES) )
Expand Down
11 changes: 7 additions & 4 deletions _studio/shared/include/libmfx_core_vaapi.h
View file
Open in desktop
@@ -1,4 +1,4 @@
// Copyright (c) 2017-2018 Intel Corporation
// Copyright (c) 2017-2019 Intel Corporation
//
// Permission is hereby granted, free of charge, to any person obtaining a copy
// of this software and associated documentation files (the "Software"), to deal
Expand Down Expand Up @@ -121,9 +121,10 @@ class VAAPIVideoCORE : public CommonCORE
CMEnabledCoreAdapter(VAAPIVideoCORE *pVAAPICore): m_pVAAPICore(pVAAPICore)
{
};
virtual mfxStatus SetCmCopyStatus(bool enable)
virtual mfxStatus SetCmCopyStatus(bool enable) override
{
return m_pVAAPICore->SetCmCopyStatus(enable);
m_pVAAPICore->SetCmCopy(enable);
return MFX_ERR_NONE;
};
protected:
VAAPIVideoCORE *m_pVAAPICore;
Expand Down Expand Up @@ -175,7 +176,9 @@ class VAAPIVideoCORE : public CommonCORE
mfxStatus GetVAService(VADisplay *pVADisplay);

// this function should not be virtual
mfxStatus SetCmCopyStatus(bool enable);
void SetCmCopy(bool enable);

bool CmCopy() const { return m_bCmCopy; }

protected:
VAAPIVideoCORE(const mfxU32 adapterNum, const mfxU32 numThreadsAvailable, const mfxSession session = NULL);
Expand Down
7 changes: 3 additions & 4 deletions _studio/shared/src/libmfx_core_vaapi.cpp
View file
Open in desktop
@@ -1,4 +1,4 @@
// Copyright (c) 2017-2018 Intel Corporation
// Copyright (c) 2017-2019 Intel Corporation
//
// Permission is hereby granted, free of charge, to any person obtaining a copy
// of this software and associated documentation files (the "Software"), to deal
Expand Down Expand Up @@ -812,8 +812,8 @@ VAAPIVideoCORE::GetVAService(

} // mfxStatus VAAPIVideoCORE::GetVAService(...)

mfxStatus
VAAPIVideoCORE::SetCmCopyStatus(bool enable)
void
VAAPIVideoCORE::SetCmCopy(bool enable)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe no sense for this function to return status

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mgonchar Is it possible to change mfxStatus CMEnabledCoreInterface::SetCmCopy(bool enable) to return void as well? Can it affect ABI of some plugins?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@onabiull : please, comment.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we wan't old plugins to remain functional, we need change interfaces by adding new ones. This would affect windows version in the first hand.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@onabiull : Does this apply to VAAPIVideoCORE or VideoCORE or both?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand Oleg correctly, CMEnabledCoreInterface is part of plugin API and so change of return value's type is prohibited.

So, rename virtual mfxStatus CMEnabledCoreInterface::SetCmCopyStatus(bool enable) to virtual mfxStatus CMEnabledCoreInterface::SetCmCopy(bool enable) is also incorrect and must be reverted.

{
m_bCmCopyAllowed = enable;
if (!enable)
Expand All @@ -824,7 +824,6 @@ VAAPIVideoCORE::SetCmCopyStatus(bool enable)
}
m_bCmCopy = false;
}
return MFX_ERR_NONE;
} // mfxStatus VAAPIVideoCORE::SetCmCopyStatus(...)

mfxStatus
Expand Down