Skip to content

Add tutorial on how to use Orbbec Astra 3D cameras#18854

Merged
opencv-pushbot merged 1 commit intoopencv:masterfrom
GArik:orbbec
Nov 19, 2020
Merged

Add tutorial on how to use Orbbec Astra 3D cameras#18854
opencv-pushbot merged 1 commit intoopencv:masterfrom
GArik:orbbec

Conversation

@GArik
Copy link
Copy Markdown
Contributor

@GArik GArik commented Nov 18, 2020

Pull Request Readiness Checklist

See details at https://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request

  • I agree to contribute to the project under Apache 2 License.
  • To the best of my knowledge, the proposed patch is not based on a code under GPL or other license that is incompatible with OpenCV
  • The PR is proposed to proper branch
  • There is reference to original bug report and related work
  • There is accuracy test, performance test and test data in opencv_extra repository, if applicable
    Patch to opencv_extra has the same branch name.
  • The feature is well documented and sample code can be built with the project CMake

@GArik GArik added the category: documentation Documentation fix or update label Nov 18, 2020
@GArik GArik requested review from asmorkalov and vpisarev November 18, 2020 18:29
Copy link
Copy Markdown
Contributor

@asmorkalov asmorkalov 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 to me 👍

Copy link
Copy Markdown
Contributor

@vpisarev vpisarev left a comment

Choose a reason for hiding this comment

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

excellent work!

Copy link
Copy Markdown
Member

@alalek alalek left a comment

Choose a reason for hiding this comment

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

Documentation looks good (except webp images).
Sample requires improvements.


maxFrames = 64

I believe it should be 1 without other options.

For example, color stream has 30 FPS, depth - 31 FPS (or 30.001 doesn't matter).
So you have 1 extra frame in 30 seconds
64 buffered frames would just grow the lag between streams of data.
After ~1800 seconds (~30 minutes) you will get full 2 seconds lag between frames.

on a wall, but the depth data makes it easy.

![Color frame](images/astra_color.webp)
![Depth frame](images/astra_depth.webp)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

.webp

Please stay conservative and use JPEG / PNG formats instead at least in "docs" area.

+430 KB

Hi-res images are usually not needed for tutorial pages.

  • use lower resolution
  • use lower quality for JPEG encoder (80 is usually good enough)

BTW, General rule is stay up to 150 Kb per image.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Is there any particular reason to avoid using webp? It is supported in all browsers except KalOS browser. I picked webp because it's much smaller than png in lossless mode.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

WebP still has low adoption in tools.
At least, GitHub PR review tool doesn't support that format.

Comment on lines +100 to +101

{ std::lock_guard<std::mutex> lk(depthFramesMtx);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please split this line

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

ok

Comment on lines +142 to +150
while (depthFrames.empty() && colorFrames.empty())
dataReady.wait(lk);

depthFramesMtx.lock();
if (depthFrames.empty())
{
depthFramesMtx.unlock();
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

How this can happen after a while() pre-check above?

Half-answer: why do we need 3 different mutexes?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The condition in the while() loop is true only when both lists are empty, so one of the lists can be empty here.

colorFramesMtx.unlock();

// Show color frame
imshow("Color", colorFrame);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This sample processes color/depth streams independently.
It is not a common flow for such applications.

They usually has some function like process(colorFrame, depthMap) which takes data from both streams.
There is no way to call processing with just one data frame.
Current "if"s logic above doesn't care about that and processes streams independently and optionally.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Indeed some post-processing is required to sync two streams. It will be implemented later, I guess. This topic is out of scope for now.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

out of scope

OK

@opencv-pushbot opencv-pushbot merged commit adafb20 into opencv:master Nov 19, 2020
@alalek alalek mentioned this pull request Nov 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants