Vp9 and cc support in samples #681

fzhar · 2018-09-27T14:49:10Z

No description provided.

dvrogozh

Please, fix your commits to make CI 100% pass.

dvrogozh · 2018-09-27T15:11:15Z

samples/sample_common/src/sample_utils.cpp

+	MSDK_CHECK_ERROR(m_bInited, false, MFX_ERR_NOT_INITIALIZED);
+	MSDK_CHECK_POINTER(pSurface, MFX_ERR_NULL_PTR);
+
+	mfxFrameInfo &pInfo = pSurface->Info;


These lines looks to be identical. Are line feeds the same? Are there tabs/space difference?

dvrogozh

Could you, please, make this change first in the series, not the last one. This will simplify review of other changes.

samples/sample_hevc_fei_abr/src/pipeline_hevc_fei.cpp

dvrogozh

This change and 3 next ones don't compile separately. Each commit should lead to compilable state otherwise we will have issues with potential bisects.

samples/sample_encode/src/sample_encode.cpp

dvrogozh · 2018-09-28T15:32:55Z

samples/sample_common/src/sample_utils.cpp

@@ -71,11 +71,12 @@ mfxStatus CopyBitstream2(mfxBitstream *dest, mfxBitstream *src)
 CSmplYUVReader::CSmplYUVReader()
 {
    m_bInited = false;
-    m_ColorFormat = MFX_FOURCC_YV12;
-    shouldShiftP010High  = false;
+    m_ColorFormat 


please, assign on a single line.

dvrogozh · 2018-09-28T15:33:10Z

samples/sample_common/src/sample_utils.cpp

@@ -86,14 +87,21 @@ mfxStatus CSmplYUVReader::Init(std::list<msdk_string> inputs, mfxU32 ColorFormat
        MFX_FOURCC_RGB4 != ColorFormat &&
        MFX_FOURCC_BGR4 != ColorFormat &&
        MFX_FOURCC_P010 != ColorFormat &&
-        MFX_FOURCC_P210 != ColorFormat)
+        MFX_FOURCC_P210 != ColorFormat &&
+		MFX_FOURCC_AYUV != ColorFormat &&


alignment is wrong

dvrogozh · 2018-09-28T15:33:25Z

samples/sample_common/src/sample_utils.cpp

                {
                    return MFX_ERR_MORE_DATA;
                }
            }
            break;
-        default:
+		case MFX_FOURCC_AYUV:


alignment is wrong

dvrogozh · 2018-09-28T15:34:30Z

samples/sample_decode/src/pipeline_decode.cpp

@@ -912,9 +914,10 @@ mfxStatus CDecodingPipeline::InitVppParams()
    }

    // P010 video surfaces should be shifted
-    if (m_mfxVppVideoParams.vpp.Out.FourCC == MFX_FOURCC_P010 && m_memType != SYSTEM_MEMORY)
+    if ((m_mfxVppVideoParams.vpp.Out.FourCC == MFX_FOURCC_P010 || m_mfxVppVideoParams.vpp.Out.FourCC == MFX_FOURCC_Y210)


Y210 is not covered by version check

still not resolved

dvrogozh · 2018-09-28T15:34:49Z

samples/sample_decode/src/pipeline_decode.cpp

    MSDK_ZERO_MEMORY(VppRequest[0]);
    MSDK_ZERO_MEMORY(VppRequest[1]);

    sts = m_pmfxDEC->Query(&m_mfxVideoParams, &m_mfxVideoParams);
    MSDK_IGNORE_MFX_STS(sts, MFX_WRN_INCOMPATIBLE_VIDEO_PARAM);
    MSDK_CHECK_STATUS(sts, "m_pmfxDEC->Query failed");

+	// Workaround for VP9 codec
+	if ((m_mfxVideoParams.mfx.FrameInfo.FourCC == MFX_FOURCC_P010 || m_mfxVideoParams.mfx.FrameInfo.FourCC == MFX_FOURCC_Y210) && m_mfxVideoParams.mfx.CodecId==MFX_CODEC_VP9)


Y210 is not covered by version check

dvrogozh · 2018-09-28T15:35:07Z

samples/sample_encode/src/pipeline_encode.cpp

-        AreGuidsEqual(pParams->pluginParams.pluginGuid, MFX_PLUGINID_HEVCE_HW);
-    bool readerShift = m_pmfxVPP ? pParams->shouldUseShiftedP010VPP  : pParams->shouldUseShiftedP010Enc;
+	bool readerShift = false;
+	if (pParams->FileInputFourCC == MFX_FOURCC_P010 || pParams->FileInputFourCC == MFX_FOURCC_P210 || pParams->FileInputFourCC == MFX_FOURCC_Y210)


Y210 is not covered by version check

this is not resolved, @fzhar. Why you closed the discussion?

samples/sample_encode/src/pipeline_encode.cpp

samples/metrics_monitor/include/cttmetrics.h

samples/sample_encode/include/pipeline_encode.h

samples/metrics_monitor/include/cttmetrics.h

samples/sample_common/src/sample_utils.cpp

samples/sample_encode/include/pipeline_encode.h

samples/sample_encode/src/sample_encode.cpp

dmitryermilov · 2018-10-02T13:41:58Z

@fzhar , any progress with addressing DmitryR concerns?

dvrogozh

@fzhar : there are few types of work which which we are doing here and few corresponding areas of responsibility:

Engineering work, i.e. implement a feature(s) and make them functional
Reviewer work, i.e. make sure that 1) feature is implemented correctly, 2) code is correctly structured, broken into commits, 3) code is "in a good shape"
Maintainer work, i.e. cherry-pick changes to release branches if needed
It is important to recognize and respect all these types of works.

As of now your commits will lead to nightmare in a few aspects:

Users will have issues in bisecting (for example, because you don't cover code with version checks initially and fix this later on)
There will be issues with cherry-picking into release branches (for example, since changes are not isolated to specific commits)
I supplied only few reasons why there will be issues. There are other reasons as well.

Could you, please, try to refactor your commits following the following guidelines:

Big non-functional changes should be isolated to separate commits. Each commit should change a code in a most trivial way. For example, if you change tabs to space then change only tabs and spaces and describe what you did in commit message. Do not attempt to change comments in the same commit - this will be unreviewable.
Prepare code before your changes: separate out pre-requisite commits which don't introduce new features, but make some preliminary job. These commits should be first in the line. For example, this can be some tabs/whitespace change commits or isomorphic code refactoring.
Address comments in the same commit where comments are given unless you explicitly asked to extracted something to a separate commit. Learn git --amend and git rebase -i HEAD~7. These are your best friends.
Answer comments, don't silently close them. If you disagree - explain your point. We have a lot of code, it is difficult to remember everything and quite easy to misread. Some comments are actually clarifying questions.
Feature changes should be structured: don't implement few significantly different changes in the same commit.
Commits should be targeted: don't change the code which you actually don't need to change. I.e. avoid useless white space changes in feature commits - this gives issues in review.

Being said that, specifically for these changes:

Commit which changes tabs to spaces - either drop it completely or pull it int he beginning of the series
Last commit which fixes something in prev. commit - it should cease to exist. This should be done in the previous commits.
Pay attention on the comments to extract bug fixes and avoid changes for which we got negative community feedback in the past.

samples/sample_common/src/sample_utils.cpp

samples/sample_decode/src/pipeline_decode.cpp

dvrogozh · 2018-10-03T16:49:17Z

samples/sample_common/src/sample_utils.cpp


-    if (MFX_FOURCC_YUY2 == pInfo.FourCC || MFX_FOURCC_RGB4 == pInfo.FourCC || MFX_FOURCC_BGR4 == pInfo.FourCC)
+    if (MFX_FOURCC_YUY2 == pInfo.FourCC || MFX_FOURCC_RGB4 == pInfo.FourCC || MFX_FOURCC_BGR4 == pInfo.FourCC || pInfo.FourCC == MFX_FOURCC_Y210 || pInfo.FourCC == MFX_FOURCC_Y410)


Y210 and Y410 should be under version check

samples/sample_common/src/sample_utils.cpp

dvrogozh · 2018-10-03T16:50:53Z

samples/sample_decode/src/pipeline_decode.cpp

@@ -912,9 +914,10 @@ mfxStatus CDecodingPipeline::InitVppParams()
    }

    // P010 video surfaces should be shifted
-    if (m_mfxVppVideoParams.vpp.Out.FourCC == MFX_FOURCC_P010 && m_memType != SYSTEM_MEMORY)
+    if ((m_mfxVppVideoParams.vpp.Out.FourCC == MFX_FOURCC_P010 || m_mfxVppVideoParams.vpp.Out.FourCC == MFX_FOURCC_Y210)


still not resolved

dvrogozh · 2018-10-03T16:54:20Z

We returned almost to the same state which was when you first committed this PR. Comments not addressed.

dmitryermilov · 2018-10-10T14:27:42Z

@dvrogozh , are your concerns addressed?

dvrogozh · 2018-10-10T17:13:17Z

@dmitryermilov : no, they were not addressed.

dvrogozh · 2018-11-01T16:54:57Z

I don't see any of the comments addressed.

onabiull

@dvrogozh your call :)

dvrogozh

Looks better with the alignment, but still PR is not structured. Please, make sure you have:

Separate commits for the bugfixes (for those features which were already available)
Separate commits for the feature enabling. If you have few significantly different features they should be in separate commits (VP9 and new color formats are significantly different features)

dvrogozh · 2018-11-07T15:59:50Z

samples/sample_common/src/sample_utils.cpp

    {
        return MFX_ERR_UNSUPPORTED;
    }

-    if(MFX_FOURCC_P010 == ColorFormat)
+    if(MFX_FOURCC_P010 == ColorFormat || MFX_FOURCC_P210 == ColorFormat || MFX_FOURCC_Y210 == ColorFormat)


if (MFX_VERSION >= 1027) check for Y210 is missed

This indeed look inconsistent. Please add guard.

samples/sample_common/src/sample_utils.cpp

dvrogozh · 2018-11-07T16:34:28Z

samples/sample_common/src/sample_utils.cpp

-            ptr = MSDK_MIN( MSDK_MIN(pData.R, pData.G), pData.B);
-            ptr = ptr + pInfo.CropX + pInfo.CropY * pData.Pitch;
+    case MFX_FOURCC_RGB4:
+    case 100: //DXGI_FORMAT_AYUV


same question as above

dvrogozh · 2018-11-07T16:36:16Z

samples/sample_common/src/sample_utils.cpp

@@ -179,20 +179,27 @@ mfxStatus CSmplYUVReader::LoadNextFrame(mfxFrameSurface1* pSurface)

    mfxU32 nBytesPerPixel = (pInfo.FourCC == MFX_FOURCC_P010 || pInfo.FourCC == MFX_FOURCC_P210 ) ? 2 : 1;

-    if (MFX_FOURCC_YUY2 == pInfo.FourCC || MFX_FOURCC_RGB4 == pInfo.FourCC || MFX_FOURCC_BGR4 == pInfo.FourCC 
+    if (   MFX_FOURCC_YUY2 == pInfo.FourCC


belongs to previous commit.

dvrogozh · 2018-11-07T16:37:12Z

samples/sample_common/src/sample_utils.cpp

@@ -2450,8 +2442,16 @@ mfxU16 FourCCToChroma(mfxU32 fourCC)
        return MFX_CHROMAFORMAT_YUV420;
    case MFX_FOURCC_NV16:
    case MFX_FOURCC_P210:
+#if (MFX_VERSION >= 1027)


belongs to prev. commit

dvrogozh · 2018-11-07T16:37:40Z

samples/sample_common/src/sample_utils.cpp

 #endif
    )
    {
        //Packed format: Luminance and chrominance are on the same plane
        switch (m_ColorFormat)
        {
+        case MFX_FOURCC_A2RGB10:
+        case MFX_FOURCC_AYUV:


don't mix YUV and RGB color formats.

Fixed. Although it was fully working code, I've separated them to make it more readable

dvrogozh · 2018-11-07T16:38:06Z

samples/sample_common/src/sample_utils.cpp

        case MFX_FOURCC_RGB4:
        case MFX_FOURCC_BGR4:
            pitch = pData.Pitch;
            ptr = MSDK_MIN( MSDK_MIN(pData.R, pData.G), pData.B);
-            ptr = ptr + pInfo.CropX + pInfo.CropY * pData.Pitch;


Separate bug fix. Attach to the next commit?

dvrogozh · 2018-11-07T16:41:12Z

samples/sample_encode/src/sample_encode.cpp

    {
        PrintHelp(strInput[0], MSDK_STRING("Unknown codec"));
        return MFX_ERR_UNSUPPORTED;
    }

-    if (MFX_CODEC_JPEG != pParams->CodecId &&
+    if (MFX_CODEC_JPEG != pParams->CodecId && MFX_CODEC_HEVC != pParams->CodecId &&


looks to be separate bug fix

It is integral part of this commit.

dvrogozh · 2018-11-07T16:41:45Z

samples/sample_encode/src/sample_encode.cpp

        return MFX_ERR_UNSUPPORTED;
    }

-    if (MFX_CODEC_HEVC != pParams->CodecId && (pParams->EncodeFourCC == MFX_FOURCC_P010) )
+    if (MFX_CODEC_HEVC != pParams->CodecId && MFX_CODEC_VP9 != pParams->CodecId && (pParams->EncodeFourCC == MFX_FOURCC_P010) )


I don't very much like intermix of VP9 support enabling with color formats. Can we have separate commit for VP9, please?

Both of them uses same common code base changes in sample_common, so they are quite tightly bound

onabiull · 2018-11-12T14:15:21Z

samples/sample_common/src/sample_utils.cpp

    {
        return MFX_ERR_UNSUPPORTED;
    }

-    if(MFX_FOURCC_P010 == ColorFormat)
+    if(MFX_FOURCC_P010 == ColorFormat || MFX_FOURCC_P210 == ColorFormat || MFX_FOURCC_Y210 == ColorFormat)


This indeed look inconsistent. Please add guard.

samples/sample_common/src/sample_utils.cpp

onabiull · 2018-11-12T14:17:06Z

samples/sample_common/src/sample_utils.cpp

-            ptr = MSDK_MIN( MSDK_MIN(pData.R, pData.G), pData.B);
-            ptr = ptr + pInfo.CropX + pInfo.CropY * pData.Pitch;
+    case MFX_FOURCC_RGB4:
+    case 100: //DXGI_FORMAT_AYUV


onabiull · 2018-11-12T14:18:10Z

samples/sample_encode/src/sample_encode.cpp

    {
        PrintHelp(strInput[0], MSDK_STRING("Unknown codec"));
        return MFX_ERR_UNSUPPORTED;
    }

-    if (MFX_CODEC_JPEG != pParams->CodecId &&
+    if (MFX_CODEC_JPEG != pParams->CodecId && MFX_CODEC_HEVC != pParams->CodecId &&


It is integral part of this commit.

samples/sample_common/src/sample_utils.cpp

Change-Id: I43225b55e8e1b0cfd69e8f156c401da2610f1463

Change-Id: I637010ff1856fa6550b64ac829a1aa55ceb09192

fzhar requested review from dmitryermilov and onabiull as code owners September 27, 2018 14:49

dvrogozh suggested changes Sep 27, 2018

View reviewed changes

samples/sample_hevc_fei_abr/src/pipeline_hevc_fei.cpp Outdated Show resolved Hide resolved

dvrogozh suggested changes Sep 27, 2018

View reviewed changes

fzhar force-pushed the VP9AndCCSupportInSamples branch from 04fdf86 to 0a96bd3 Compare September 28, 2018 07:26

dvrogozh suggested changes Sep 28, 2018

View reviewed changes

dvrogozh suggested changes Oct 1, 2018

View reviewed changes

dvrogozh suggested changes Oct 3, 2018

View reviewed changes

samples/sample_common/src/sample_utils.cpp Show resolved Hide resolved

samples/sample_common/src/sample_utils.cpp Show resolved Hide resolved

samples/sample_decode/src/pipeline_decode.cpp Show resolved Hide resolved

fzhar force-pushed the VP9AndCCSupportInSamples branch from 2595f79 to c12aeeb Compare October 3, 2018 16:36

dvrogozh suggested changes Oct 3, 2018

View reviewed changes

fzhar force-pushed the VP9AndCCSupportInSamples branch from c12aeeb to 7beba65 Compare October 4, 2018 17:37

fzhar force-pushed the VP9AndCCSupportInSamples branch from 7beba65 to 662d133 Compare October 29, 2018 16:03

fzhar force-pushed the VP9AndCCSupportInSamples branch 3 times, most recently from ca60cd2 to a50a30f Compare November 7, 2018 14:03

onabiull approved these changes Nov 7, 2018

View reviewed changes

dvrogozh suggested changes Nov 7, 2018

View reviewed changes

onabiull suggested changes Nov 12, 2018

View reviewed changes

fzhar force-pushed the VP9AndCCSupportInSamples branch from a50a30f to ebab829 Compare November 12, 2018 16:21

onabiull previously approved these changes Nov 12, 2018

View reviewed changes

dvrogozh suggested changes Nov 12, 2018

View reviewed changes

samples/sample_common/src/sample_utils.cpp Show resolved Hide resolved

dmitryermilov and others added 2 commits November 13, 2018 16:01

VP9 Support. New color formats support.

8eb54dd

Change-Id: I43225b55e8e1b0cfd69e8f156c401da2610f1463

Error in offset calculation for YUY2 is fixed

5cfdc76

Change-Id: I637010ff1856fa6550b64ac829a1aa55ceb09192

fzhar dismissed onabiull’s stale review via 5cfdc76 November 13, 2018 13:02

fzhar force-pushed the VP9AndCCSupportInSamples branch from ebab829 to 5cfdc76 Compare November 13, 2018 13:02

onabiull approved these changes Nov 13, 2018

View reviewed changes

onabiull merged commit 38f39b4 into master Nov 13, 2018

onabiull deleted the VP9AndCCSupportInSamples branch November 13, 2018 14:28


		if (MFX_FOURCC_YUY2 == pInfo.FourCC \|\| MFX_FOURCC_RGB4 == pInfo.FourCC \|\| MFX_FOURCC_BGR4 == pInfo.FourCC)
		if (MFX_FOURCC_YUY2 == pInfo.FourCC \|\| MFX_FOURCC_RGB4 == pInfo.FourCC \|\| MFX_FOURCC_BGR4 == pInfo.FourCC \|\| pInfo.FourCC == MFX_FOURCC_Y210 \|\| pInfo.FourCC == MFX_FOURCC_Y410)

Vp9 and cc support in samples #681

Vp9 and cc support in samples #681

Conversation

fzhar commented Sep 27, 2018

dvrogozh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dvrogozh left a comment

Choose a reason for hiding this comment

dvrogozh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmitryermilov commented Oct 2, 2018

dvrogozh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dvrogozh commented Oct 3, 2018

dmitryermilov commented Oct 10, 2018

dvrogozh commented Oct 10, 2018

dvrogozh commented Nov 1, 2018

onabiull left a comment

Choose a reason for hiding this comment

dvrogozh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment