Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add description of the return status for vaSyncSurface() #774

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

pengxin99
Copy link
Contributor

this will indicate APP behavior when meet VA_STATUS_ERROR_DECODING_ERROR from vaSyncSurface

@pengxin99 pengxin99 force-pushed the pengxin/add_comments_for_vasyncsurface branch from 79c3709 to 8ba1827 Compare November 27, 2023 10:06
@pengxin99 pengxin99 changed the title Add explaination of decoding error status for vaSyncSurfac Add description of decoding error status for vaSyncSurface() Nov 27, 2023
@pengxin99 pengxin99 force-pushed the pengxin/add_comments_for_vasyncsurface branch from 8ba1827 to 36407ae Compare November 27, 2023 11:08
va/va.h Outdated Show resolved Hide resolved
@pengxin99 pengxin99 requested a review from Jexu December 11, 2023 10:23
@pengxin99 pengxin99 force-pushed the pengxin/add_comments_for_vasyncsurface branch from 8b94cd0 to 35d5b95 Compare December 15, 2023 07:23
@pengxin99
Copy link
Contributor Author

Hi, @XinfengZhang @Jexu, have add more detail description, could you pls help to review this PR again?

@pengxin99 pengxin99 changed the title Add description of decoding error status for vaSyncSurface() Add description of the return status for vaSyncSurface() Dec 21, 2023
va/va.h Outdated
* and verify its validity and integrity.
* - \ref VA_STATUS_ERROR_OPERATION_FAILED: When the attempt to retrieve the status report fails or related work
* on the current surface fails, this error is generated. In such cases, the client can proceed with subsequent
* frames by ignore the error, but it may involve incorrect result like corruption.
Copy link
Contributor

Choose a reason for hiding this comment

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

it means "operation failed" could be ignored by application

Copy link
Contributor Author

Choose a reason for hiding this comment

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

VA_STATUS_ERROR_OPERATION_FAILED is caused by status report fail which cannot be used to determine whether the result is correct. So declare here that APP can ignore this error but may meet incorrect result.

And will add more detail like "To init new context to avoid protentional incorrect result."

va/va.h Outdated
* on the current surface fails, this error is generated. In such cases, the client can proceed with subsequent
* frames by ignore the error, but it may involve incorrect result like corruption.
* - \ref VA_STATUS_ERROR_HW_BUSY: This error indicates that synchronization is still in progress or GPU hang happens.
* The client could call the function again within a period of time to wait hw execution completion.
Copy link
Contributor

Choose a reason for hiding this comment

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

same surfaceID again?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, will add it

va/va.h Outdated
* - \ref VA_STATUS_ERROR_HW_BUSY: This error indicates that synchronization is still in progress or GPU hang happens.
* The client could call the function again within a period of time to wait hw execution completion.
* If hw execution could not complete successfully, gpu hang and reset may occur, in this case,
* application could try to recover media context by initialing new one.
Copy link
Contributor

Choose a reason for hiding this comment

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

must recreate context? could application just ignore it?

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 think the recreate context is must, VA_STATUS_ERROR_HW_BUSY indicate that HW reset happened, this need to recover media context. Media reset will not return this error.

va/va.h Outdated
* - \ref VA_STATUS_ERROR_DECODING_ERROR: This error is encountered when macroblock (MB) errors with
* non-conformance input clips are detected during the decoding process. The application can proceed with
* subsequent frames, but it is advised to be aware of potential corruption in the output.
* Call vaQuerySurfaceError could get more details of mb error information.
Copy link
Contributor

Choose a reason for hiding this comment

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

difference comparing with VA_STATUS_ERROR_OPERATION_FAILED

@XinfengZhang
Copy link
Contributor

I agree with the direction, provide hint to application , then application could handle correlated errors , ignore , or reset the context etc. .
but I suggest to clarify the difference. currently:
VA_STATUS_ERROR_DECODING_ERROR may caused by a bistream corruption. related with syntax
VA_STATUS_ERROR_HW_BUSY is about the HW reset , but looks could not distinguish , media reset or gpu reset. if media reset , it could be ignored directly. it also related with #714?

but operation failed is unclear.

@pengxin99
Copy link
Contributor Author

pengxin99 commented Jan 12, 2024

@XinfengZhang Thanks for the comments. and I will refine this PR for below two point:

VA_STATUS_ERROR_HW_BUSY is about the HW reset , but looks could not distinguish , media reset or gpu reset. if media reset , it could be ignored directly. it also related with #714?

The VA_STATUS_ERROR_HW_BUSY is only for HW reset, so cannot ignore this error, vaSyncSurface will return success if media reset happended.

but operation failed is unclear.

can see the below comment: #774 (comment)

va/va.h Outdated
* - \ref VA_STATUS_ERROR_OPERATION_FAILED: When the attempt to retrieve the status report fails or related work
* on the current surface fails, this error is generated. In such cases, the client can proceed with subsequent
* frames by ignore the error, but it may involve incorrect result like corruption, MD5 mismatch or HW hang.
* Initialize new context can avoid the potential issues.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* Initialize new context can avoid the potential issues.
* Initializing new context can avoid the potential issues.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thx, fixed

va/va.h Outdated
* - \ref VA_STATUS_ERROR_HW_BUSY: This error indicates that synchronization is still in progress or GPU hang happens.
* The client could call the function again with the same render_target within a period of time to wait hw execution completion.
* If hw execution could not complete successfully, gpu hang and reset may occur, in this case,
* application could try to recover media context by initialing new one.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* application could try to recover media context by initialing new one.
* application could try to recover media context by initializing new one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thx, fixed

va/va.h Outdated
* - \ref VA_STATUS_ERROR_DECODING_ERROR: This error is encountered when macroblock (MB) errors with
* non-conformance input clips are detected during the decoding process. The application can proceed with
* subsequent frames, but it is advised to be aware of potential corruption in the output.
* Call vaQuerySurfaceError could get more details of mb error information.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* Call vaQuerySurfaceError could get more details of mb error information.
* Call vaQuerySurfaceError to get more details on mb error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thx, fixed

va/va.h Outdated
@@ -4187,6 +4187,26 @@ Synchronization
* This function blocks until all pending operations on the render target
* have been completed. Upon return it is safe to use the render target for a
* different picture.
*
* Possible errors:
* - \ref VA_STATUS_ERROR_INVALID_CONTEXT: This error occurs when the function is provided with an invalid context.
Copy link
Contributor

Choose a reason for hiding this comment

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

This error occurs when the function is provided with an invalid context.

Function prototype does not have context at all. So, this error is inadequate ( VA_STATUS_ERROR_INVALID_DISPLAY seems more reasonable) or at least incorrectly described. VAStatus vaSyncSurface(VADisplay dpy, VASurfaceID render_target).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks for this advice, the media-driver will return VA_STATUS_ERROR_INVALID_CONTEXT instead of VA_STATUS_ERROR_INVALID_DISPLAY , so I only correct the comments here to make sure the API compatible with the old version.

@dvrogozh
Copy link
Contributor

Also, please, squash commits. For this change there should be just 1 commit in the PR.

this will indicate APP behavior when meet VA_STATUS_ERROR_DECODING_ERROR from vaSyncSurface

Signed-off-by: Pengxin Yuan <[email protected]>
@pengxin99 pengxin99 force-pushed the pengxin/add_comments_for_vasyncsurface branch from 3a201cd to b0a8b63 Compare January 26, 2024 02:32
@pengxin99
Copy link
Contributor Author

pengxin99 commented Jan 26, 2024

Also, please, squash commits. For this change there should be just 1 commit in the PR.

@dvrogozh Thanks for your review, and have refined this PR, could pls have a look again?

@pengxin99
Copy link
Contributor Author

Hi, @dvrogozh @XinfengZhang , Can you take another look at this PR if you have time?

Copy link
Contributor

@dvrogozh dvrogozh left a comment

Choose a reason for hiding this comment

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

looks good

@pengxin99
Copy link
Contributor Author

Hi, @XinfengZhang , Could you pls take another look at this PR when you have time?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants