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

allow ROI rectangle top/left/bottom/right to be 0 for query #1856

Closed
wants to merge 1 commit into from

Conversation

guoyejun
Copy link
Contributor

It is nature to fill the ROI rect with zero when query the max
supported ROI numbers. Without this change, the query does not
work for such case.

changed++;
i--;
continue;
if (!(ROI->ROI[i].Top == 0 && ROI->ROI[i].Left == 0 && ROI->ROI[i].Right == 0 && ROI->ROI[i].Bottom == 0)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@guoyejun , HEVC encode got a new design. Please extend the change for it. Seems this is the right place for it:

auto IsInvalidRect = [](const ROI::RectData& rect)
.

Copy link
Contributor

Choose a reason for hiding this comment

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

@guoyejun Dmitry meant to introduce your change for both codebases. Also @eistarov could you look at this one?

Copy link
Contributor

Choose a reason for hiding this comment

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

Need to separate ROI check behavior in Query and run-time - not sure about driver reaction on all-zero ROI.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@eistarov good point, i'm afraid i have no time for such change, just pass the ball to msdk team, thanks.

Copy link
Contributor

Choose a reason for hiding this comment

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

@lakulako , @saosipov , can you please help to finalize the patch?

It is nature to fill the ROI rect with zero when query the max
supported ROI numbers. Without this change, the query does not
work for such case.
Copy link
Contributor

@mgonchar mgonchar left a comment

Choose a reason for hiding this comment

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

I believe we need comment in code to describe why we have such check

Comment on lines 108 to 115
auto IsInvalidRect = [](const ROI::RectData& rect)
{
if (rect.Top == 0 && rect.Left == 0 && rect.Right == 0 && rect.Bottom == 0)
{
return false;
}
return ((rect.Left >= rect.Right) || (rect.Top >= rect.Bottom));
};
Copy link
Contributor

Choose a reason for hiding this comment

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

After CheckAndFixRect above we have here top/left aligned down and right/bottom aligned up for the cases when they are not aligned to BlockSize and right/bottom > top/left.
If top/left == right/bottom or top/left > right/bottom inside the same Block we would have top/left==right/bottom here and if top/left > right/bottom and they don't belong to the same Block we would have top/left > right/bottom.
Let me know if you don't agree with it?
So we have to consider a Rect as invalid only the way it was done before i.e. ((rect.Left >= rect.Right) || (rect.Top >= rect.Bottom))

@guoyejun
Copy link
Contributor Author

as mentioned above that "@eistarov good point, i'm afraid i have no time for such change, just pass the ball to msdk team, thanks."

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

Successfully merging this pull request may close these issues.

6 participants