Skip to content

Stickers#1958

Open
apjagdale wants to merge 6 commits into
Samsung:masterfrom
apjagdale:stickers
Open

Stickers#1958
apjagdale wants to merge 6 commits into
Samsung:masterfrom
apjagdale:stickers

Conversation

@apjagdale

Copy link
Copy Markdown
Contributor

Added screenshot capture feature by using 3 Pixel Buffer objects.
New class introduced : GVRSticker(GVRContext gvrContext, String tag, long interval)

  • tag for outputting PNG's to the SD Card along with appended frame ID in folder GearVRFScreenshots
  • (interval) Time in milliseconds to take screenshot at that interval

Methods:

  • start/stopCapturing()

GearVRf DCO signed off by: Abhijit Jagdale a.jagdale@samsung.com

@roshanch

roshanch commented Aug 6, 2018

Copy link
Copy Markdown
Contributor

We are using horizontal/ vertical flip shaders, or post effects just for inverting a scene. we should be modifying mvp instead to rotate or flip the scene instead of all that overhead which is not efficient. Which will also avoid creating unnecessary render targets.

@liaxim

liaxim commented Aug 7, 2018

Copy link
Copy Markdown
Contributor

Couple of tests I believe would be useful.
There is a backend adapter that no longer compiles.

File sdcard = Environment.getExternalStorageDirectory();
mDirectory = sdcard.getAbsolutePath() + "/GearVRFScreenshots/";
File d = new File(mDirectory);
d.mkdirs();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Probably the fact that WRITE_EXTERNAL_STORAGE is required should be mentioned. Also I'd recommend checking the return value of mkdirs and throwing an exception immediately if needed.

@liaxim

liaxim commented Aug 7, 2018

Copy link
Copy Markdown
Contributor

Have you verified screenshotting with all backend adapters?


renderTarget.cullFromCamera(mMainScene, mainCameraRig.getCenterCamera(), mRenderBundle.getShaderManager());
captureCenterEye(renderTarget, false);
captureSticker();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What about other backend adapters?

@liaxim

liaxim commented Aug 7, 2018

Copy link
Copy Markdown
Contributor

What about generating stickers in "headless mode"? Where the framework is only used to render into the images as a background "service"? Should there be all new backend adapter for that mode?


GLuint* &pbos = gRenderer->getPBOs();
if(pbos == NULL){
pbos = new GLuint[3];

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please use a constant for the number of pbos.

}
return nullptr;
}
virtual bool readRenderResultInPBO(int) = 0;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Not a big deal but PBO is not directly applicable to Vulkan, correct? A more generic name like buffer may be better.

@liaxim

liaxim commented Aug 7, 2018

Copy link
Copy Markdown
Contributor

Looks pretty good overall.

@liaxim

liaxim commented Aug 13, 2018

Copy link
Copy Markdown
Contributor

Needs rebase. Are any of the comments valid?

@NolaDonato

Copy link
Copy Markdown
Contributor

Please generalize beyond stickers. This should be a component you attach to the Scene object that owns the render target to capture textures.

@apjagdale

apjagdale commented Aug 14, 2018

Copy link
Copy Markdown
Contributor Author

@thomasflynn @NolaDonato In the current approach there is a way to set the interval at which the sticker(screenshot) needs to be captured. So if we move to RenderTarget based approach, onDrawFrame will be called every time. That means no interval but capturing every frame. Do we want interval for RenderTarget approach as well?

@NolaDonato

Copy link
Copy Markdown
Contributor

Yes that would be useful. We should be able to specify it in terms of frames or seconds

@liaxim

liaxim commented Nov 28, 2018

Copy link
Copy Markdown
Contributor

Should we close?

@NolaDonato

Copy link
Copy Markdown
Contributor

We still want the feature but it can go into SXR instead of GVRF

@ftservice ftservice left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Android system webviwver

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