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

citra_qt: camera integration #3566

Merged
merged 53 commits into from May 11, 2018

Conversation

Projects
None yet
@zhaowenlan1779
Member

zhaowenlan1779 commented Mar 24, 2018

Depends on citra-emu/ext-windows-bin#16 for QtMultimedia library

This is my first time contributing code. Hello Citra team.

Well, Hi, this one is an (almost) fine camera integration.

Over one month ago, I asked some questions on the Citra forum, and someone told me that no one had time to implement the camera. So I tried to do it.

Features:

  • OpenCVCamera supporting video files(.mpg .avi), Image sequences & system camera [Removed]
  • QtMultimediaCamera supporting system camera
  • Edited version of StillImageCamera by @wwylele (I added an option 'Prompt before load'. It allows the user to choose different images every time the camera initializes and is very useful. Also, I extracted some functions to CameraUtil and used them in other camera implementations)
  • 'Camera' Configuration tab with a preview area (preview is stopped when anything is changed):
    37860901-150b19a2-2f6a-11e8-882a-109674bc12c1
    tmp
  • To implement: System camera selection

I hardly have time on Monday-Friday. If you have any ideas, you may leave comments.

Thanks!


This change is Reviewable

@cluezbot

This comment has been minimized.

cluezbot commented Mar 24, 2018

Hi, this is neobot, using neobrain's account. I'm keeping an archive of versions of this PR:

2018-03-24T10:39:48Z: 327ad8b...zhaowenlan1779:f87021bfdfcca8fa93df40c9996e0b3620831ed2

@wwylele

This comment has been minimized.

Member

wwylele commented Mar 24, 2018

Hello, thanks for the contribution! The first glance over the code looks good, and I will do a thorough review next week. However we currently have several large projects ongoing (multiplayer, pica->glsl, yuzu) so the review process might be slow. I apologize for it in advance.

Regarding OpenCV, can we make it an optional dependency, as well as its camera implementation? So that devs don't need to download/compile it every time developing citra.

You may also want @yuriks or @Subv to review the service management change to ensure it doesn't go against the design idea.

@cluezbot

This comment has been minimized.

cluezbot commented Mar 24, 2018

Hi, this is neobot, using neobrain's account. I'm keeping an archive of versions of this PR:

2018-03-24T12:19:57Z: 327ad8b...zhaowenlan1779:8668c1782db879a6805cb23a6c002e19be3f26f2

@zhaowenlan1779

This comment has been minimized.

Member

zhaowenlan1779 commented Mar 24, 2018

I've made OpenCVCamera optional with an ENABLE_OPENCV option. It is turned off if Qt isn't enabled, by the way.

@cluezbot

This comment has been minimized.

cluezbot commented Mar 24, 2018

Hi, this is neobot, using neobrain's account. I'm keeping an archive of versions of this PR:

2018-03-24T12:56:05Z: 327ad8b...zhaowenlan1779:14af325cda79240330b145de5f7eb6efc03d0cc7

@cluezbot

This comment has been minimized.

cluezbot commented Mar 24, 2018

Hi, this is neobot, using neobrain's account. I'm keeping an archive of versions of this PR:

2018-03-24T13:02:03Z: 327ad8b...zhaowenlan1779:97d210cd17c1d0b4d09806785428922eff4258f3

@zhaowenlan1779 zhaowenlan1779 force-pushed the zhaowenlan1779:camera branch from 97d210c to 508ccca Mar 24, 2018

@cluezbot

This comment has been minimized.

cluezbot commented Mar 24, 2018

Hi, this is neobot, using neobrain's account. I'm keeping an archive of versions of this PR:

2018-03-24T13:28:33Z: 327ad8b...zhaowenlan1779:508cccaed111a67ae51faf190c449654d72d7f1c

@zhaowenlan1779

This comment has been minimized.

Member

zhaowenlan1779 commented Mar 24, 2018

Update: Rebased into 3 commits.

@cluezbot

This comment has been minimized.

cluezbot commented Mar 24, 2018

Hi, this is neobot, using neobrain's account. I'm keeping an archive of versions of this PR:

2018-03-24T23:02:42Z: 9283053...zhaowenlan1779:a560ea69a0eea0b1338827a60e8d3e76e078c02a

@cluezbot

This comment has been minimized.

cluezbot commented Mar 24, 2018

Hi, this is neobot, using neobrain's account. I'm keeping an archive of versions of this PR:

2018-03-24T23:45:06Z: 9283053...zhaowenlan1779:9af52b25a50795e216527c98b9cdc97ad8cf58d6

@cluezbot

This comment has been minimized.

cluezbot commented Mar 25, 2018

Hi, this is neobot, using neobrain's account. I'm keeping an archive of versions of this PR:

2018-03-25T00:21:36Z: 9283053...zhaowenlan1779:fba3289b2dd43ab74bc7f248e5e5e262d296ef57

@cluezbot

This comment has been minimized.

cluezbot commented Mar 25, 2018

Hi, this is neobot, using neobrain's account. I'm keeping an archive of versions of this PR:

2018-03-25T00:41:01Z: 9283053...zhaowenlan1779:8792f3b6818c9686d1c27f8cd6931bf71c324072

@cluezbot

This comment has been minimized.

cluezbot commented Mar 25, 2018

Hi, this is neobot, using neobrain's account. I'm keeping an archive of versions of this PR:

2018-03-25T00:54:13Z: 9283053...zhaowenlan1779:7923d5d942315a5d1df11f4b7fcb38afad2b9a1c

@cluezbot

This comment has been minimized.

cluezbot commented Mar 25, 2018

Hi, this is neobot, using neobrain's account. I'm keeping an archive of versions of this PR:

2018-03-25T02:14:04Z: 9283053...zhaowenlan1779:f7e4a5d278341267475450f2e25d04fb99dee3ba

@zhaowenlan1779 zhaowenlan1779 force-pushed the zhaowenlan1779:camera branch from 7923d5d to f7e4a5d Mar 25, 2018

@cluezbot

This comment has been minimized.

cluezbot commented Mar 25, 2018

Hi, this is neobot, using neobrain's account. I'm keeping an archive of versions of this PR:

2018-03-25T02:47:40Z: 9283053...zhaowenlan1779:343b03911b73c65d7d68e32c6abb210ce220a2f1

@cluezbot

This comment has been minimized.

cluezbot commented Mar 25, 2018

Hi, this is neobot, using neobrain's account. I'm keeping an archive of versions of this PR:

2018-03-25T03:20:10Z: 9283053...zhaowenlan1779:f3f07a458ddaf119f8037d44bbe67c727bab5fa6

@cluezbot

This comment has been minimized.

cluezbot commented Mar 25, 2018

Hi, this is neobot, using neobrain's account. I'm keeping an archive of versions of this PR:

2018-03-25T03:50:37Z: 9283053...zhaowenlan1779:ddaca1fdb42be7ddd7613ac0dab62223eb11e059

@zhaowenlan1779

This comment has been minimized.

Member

zhaowenlan1779 commented Mar 25, 2018

OK. This looks fine. But CI takes a little more time to compile OpenCV from source.
I won't have time on Mon-Fri.
You may edit the code yourself if you have any ideas.

@@ -3,17 +3,39 @@ set(CMAKE_AUTORCC ON)
set(CMAKE_INCLUDE_CURRENT_DIR ON)
set(CMAKE_MODULE_PATH ${CMAKE_MODULE_PATH} ${PROJECT_SOURCE_DIR}/CMakeModules)
if (ENABLE_OPENCV)

This comment has been minimized.

@lioncash

lioncash Mar 25, 2018

Member

If you place this block after the add_executable you can just do target_sources(citra-qt PRIVATE <source files here>)

similarly add_definitions can be turned into target_compile_definitions(citra-qt PRIVATE -DENABLE_OPENCV_CAMERA)

236, 237, 237, 238, 238, 239, 239, 240, 240,
};
inline int U(int r, int g, int b) {

This comment has been minimized.

@lioncash

lioncash Mar 25, 2018

Member

This can be static constexpr instead of inline.

53, 53, 53, 53, 53, 53, 53, 53, 54, 54, 54, 54, 54, 54, 54, 54,
};
inline int Y(int r, int g, int b) {

This comment has been minimized.

@lioncash

lioncash Mar 25, 2018

Member

This can be static constexpr instead of inline

// The following is data table for RGB -> YUV conversions.
namespace YuvTable {
const int Y_R[256] = {

This comment has been minimized.

@lioncash

lioncash Mar 25, 2018

Member

All of these arrays can be constexpr std::array`s

namespace CameraUtil {
// The following is data table for RGB -> YUV conversions.

This comment has been minimized.

@lioncash

lioncash Mar 25, 2018

Member

are data tables

previewing_camera =
Camera::CreateCameraPreview(camera_name[camera_selection], camera_config[camera_selection]);
if (previewing_camera) {

This comment has been minimized.

@lioncash

lioncash Mar 25, 2018

Member
if (!previewing_camera) {
    return;
}

The rest of the code can then be unindented.

this->setConfiguration();
}
ConfigureCamera::~ConfigureCamera() {}

This comment has been minimized.

@lioncash

lioncash Mar 25, 2018

Member
ConfigureCamera::~ConfigureCamera() = default;
@@ -0,0 +1,264 @@
// Copyright 2016 Citra Emulator Project

This comment has been minimized.

@lioncash
std::unique_ptr<CameraInterface> StillImageCameraFactory::CreatePreview(
const std::string& config) const {
std::string real_config = config;
if (config == "") {

This comment has been minimized.

@lioncash

lioncash Mar 25, 2018

Member

if (config.empty())

const std::string StillImageCameraFactory::getFilePath() {
QList<QByteArray> types = QImageReader::supportedImageFormats();
QList<QString> temp_filters = {};

This comment has been minimized.

@lioncash

lioncash Mar 25, 2018

Member

= {}; is redundant.

@jroweboy

This comment has been minimized.

Member

jroweboy commented Mar 27, 2018

Hello! Thanks a bunch for your first contribution; its a big one! I read over the code, and I have a few concerns that I'd like to hear from you about before I do a full code review.

  • I left some design feedback in the previous PR about the expected use case. Would really like to discuss this either in this PR or in that issue, but I'm currently not happy that we are trying to get users to play a game of guess-a-camera when configuring the camera.
  • I'm really nervous about adding opencv to the build. Our build times on appveyor are already astronomical (30 minutes for mingw!!) Are you sure that we can't get QCamera to work? I ran through the docs and it seems to be everything we need.

If you need any direct help, please hop in #citra-dev on freenode, or stop by the citra discord and say hi. Thanks again for the contribution!

@cluezbot

This comment has been minimized.

cluezbot commented Apr 29, 2018

Hi, this is neobot, using neobrain's account. I'm keeping an archive of versions of this PR:

2018-04-29T09:59:10Z: c8d4ca8...zhaowenlan1779:02c4c1de5173db2f780aa1a97866e22f436fce2b

@cluezbot

This comment has been minimized.

cluezbot commented Apr 29, 2018

Hi, this is neobot, using neobrain's account. I'm keeping an archive of versions of this PR:

2018-04-29T10:03:46Z: c8d4ca8...zhaowenlan1779:8c26b78fec965b8bce8150fe0fb1ab8748b61d09

@zhaowenlan1779

This comment has been minimized.

Member

zhaowenlan1779 commented Apr 29, 2018

@jmorriz124 Tested on:
latest canary (56e361b): Issue reproduced
latest nightly (9c65a45): Cannot reproduce issue
camera dev branch (camera-8c26b78): Issue reproduced
Looks like this really has something to do with my PR, but I don't think I introduced anything strange.

@cluezbot

This comment has been minimized.

cluezbot commented Apr 29, 2018

Hi, this is neobot, using neobrain's account. I'm keeping an archive of versions of this PR:

2018-04-29T12:54:58Z: c8d4ca8...zhaowenlan1779:29e03c0679853d5a6a0e790fd5df66bbf5e9bbe4

@zhaowenlan1779

This comment has been minimized.

Member

zhaowenlan1779 commented Apr 29, 2018

@jmorriz124 Issue fixed. Thank you for your report.

@cluezbot

This comment has been minimized.

cluezbot commented Apr 30, 2018

Hi, this is neobot, using neobrain's account. I'm keeping an archive of versions of this PR:

2018-04-30T03:02:39Z: c8d4ca8...zhaowenlan1779:e2a159b3c25fc885ef94e63936b6e5117f64a8a7

@zhaowenlan1779

This comment has been minimized.

Member

zhaowenlan1779 commented May 2, 2018

@jroweboy @wwylele Any feedback/issues? If no, I wonder when we can get this merged into Nightly..

@wwylele

wwylele approved these changes May 2, 2018

@valentinvanelslande

This comment has been minimized.

Contributor

valentinvanelslande commented May 3, 2018

@zhaowenlan1779 I created a PR in your fork

zhaowenlan1779 added some commits May 6, 2018

@jroweboy jroweboy merged commit 57827de into citra-emu:master May 11, 2018

2 of 3 checks passed

code-review/reviewable 28 files, 108 discussions left (lioncash, valentinvanelslande, wwylele, zhaowenlan1779)
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@zhaowenlan1779 zhaowenlan1779 deleted the zhaowenlan1779:camera branch May 12, 2018

@Leo626

This comment has been minimized.

Leo626 commented May 13, 2018

@zhaowenlan1779 Apologies for bumping this but it seems that System Camera is mirrored for me.

citra-qt_2018-05-13_12-52-19

@zhaowenlan1779

This comment has been minimized.

Member

zhaowenlan1779 commented May 19, 2018

@Leo626 Well, I see. Will fix

@zhaowenlan1779

This comment has been minimized.

Member

zhaowenlan1779 commented May 19, 2018

@Leo626 You had better open an issue

@Leo626

This comment has been minimized.

Leo626 commented May 20, 2018

@zhaowenlan1779 I opened the issue (#3763).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment