Message ID | 20200502021045.785979-4-niklas.soderlund@ragnatech.se |
---|---|
State | Superseded |
Headers | show |
Series |
|
Related | show |
Hi Niklas, Thank you for the patch. On Sat, May 02, 2020 at 04:10:45AM +0200, Niklas Söderlund wrote: > Add a toolbar button that captures RAW data to disk. The button is only > enabled if the camera is configured to provide a raw stream to the > application. > > Only when the capture action is triggered will a request with a raw > buffer be queued to the camera. > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > --- > * Changes since v2 > - Use a file dialog > - Make DNG optional depending on libtiff > --- > src/qcam/assets/feathericons/feathericons.qrc | 1 + > src/qcam/main_window.cpp | 64 ++++++++++++++++++- > src/qcam/main_window.h | 4 ++ > 3 files changed, 68 insertions(+), 1 deletion(-) > > diff --git a/src/qcam/assets/feathericons/feathericons.qrc b/src/qcam/assets/feathericons/feathericons.qrc > index c4eb7a0be6884373..fc8213928ece70ea 100644 > --- a/src/qcam/assets/feathericons/feathericons.qrc > +++ b/src/qcam/assets/feathericons/feathericons.qrc > @@ -1,5 +1,6 @@ > <!DOCTYPE RCC><RCC version="1.0"> > <qresource> > +<file>./aperture.svg</file> > <file>./camera-off.svg</file> > <file>./play-circle.svg</file> > <file>./save.svg</file> > diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp > index 0bd9f3583ea4f6d4..458da479a9b21b73 100644 > --- a/src/qcam/main_window.cpp > +++ b/src/qcam/main_window.cpp > @@ -27,6 +27,10 @@ > #include <libcamera/camera_manager.h> > #include <libcamera/version.h> > > +#if HAVE_TIFF I wonder if this shouldn't be called HAVE_DNG, as the dependency on libtiff is internal to the DNGWriter class. > +#include "dng_writer.h" > +#endif > + > using namespace libcamera; > > /** > @@ -48,7 +52,8 @@ public: > }; > > MainWindow::MainWindow(CameraManager *cm, const OptionsParser::Options &options) > - : options_(options), cm_(cm), allocator_(nullptr), isCapturing_(false) > + : saveRaw_(nullptr), options_(options), cm_(cm), allocator_(nullptr), > + isCapturing_(false), captureRaw_(false) > { > int ret; > > @@ -144,6 +149,16 @@ int MainWindow::createToolbars() > action->setShortcut(QKeySequence::SaveAs); > connect(action, &QAction::triggered, this, &MainWindow::saveImageAs); > > +#if HAVE_TIFF > + /* Save Raw action. */ > + action = toolbar_->addAction(QIcon::fromTheme("camera-photo", > + QIcon(":aperture.svg")), > + "Save Raw"); > + action->setEnabled(false); > + connect(action, &QAction::triggered, this, &MainWindow::captureRaw); > + saveRaw_ = action; > +#endif I wonder if we should still have the button, but never enable it. If we don't, users may wonder why the button isn't there. But if we do, they may wonder why it's never enabled :-) Maybe the tooltip text could be set to - "RAW capture not available (missing libtiff)" - "RAW capture not enabled on the command line" - "Save RAW" depending on the situation. Or maybe all of this is overkill :-) > + > return 0; > } > > @@ -369,6 +384,10 @@ int MainWindow::startCapture() > > adjustSize(); > > + /* Configure the raw capture button. */ > + if (saveRaw_) > + saveRaw_->setEnabled(config_->size() == 2); > + > /* Allocate and map buffers. */ > allocator_ = new FrameBufferAllocator(camera_); > for (StreamConfiguration &config : *config_) { > @@ -474,6 +493,9 @@ void MainWindow::stopCapture() > return; > > viewfinder_->stop(); > + if (saveRaw_) > + saveRaw_->setEnabled(false); > + captureRaw_ = false; > > int ret = camera_->stop(); > if (ret) > @@ -524,6 +546,31 @@ void MainWindow::saveImageAs() > writer.write(image); > } > > +void MainWindow::captureRaw() > +{ > + captureRaw_ = true; > +} > + > +void MainWindow::processRaw(FrameBuffer *buffer) > +{ > +#if HAVE_TIFF > + QString defaultPath = QStandardPaths::writableLocation(QStandardPaths::PicturesLocation); > + QString filename = QFileDialog::getSaveFileName(this, "Save DNG", defaultPath, > + "DNG Files (*.dng)"); > + > + if (filename != "") { if (!filename.isEmpty()) { > + const MappedBuffer &mapped = mappedBuffers_[buffer]; > + DNGWriter::write(filename.toStdString().c_str(), camera_.get(), > + rawStream_->configuration(), buffer, > + mapped.memory); > + } > +#endif Blank line here ? > + { > + QMutexLocker locker(&mutex_); > + freeBuffers_[rawStream_].enqueue(buffer); > + } > +} > + > /* ----------------------------------------------------------------------------- > * Request Completion Handling > */ > @@ -566,6 +613,9 @@ void MainWindow::processCapture() > /* Process buffers. */ > if (buffers.count(vfStream_)) > processViewfinder(buffers[vfStream_]); > + > + if (buffers.count(rawStream_)) > + processRaw(buffers[rawStream_]); > } > > void MainWindow::processViewfinder(FrameBuffer *buffer) > @@ -598,5 +648,17 @@ void MainWindow::queueRequest(FrameBuffer *buffer) > > request->addBuffer(vfStream_, buffer); > > + if (captureRaw_) { > + QMutexLocker locker(&mutex_); > + > + if (!freeBuffers_[rawStream_].isEmpty()) { > + request->addBuffer(rawStream_, > + freeBuffers_[rawStream_].dequeue()); > + captureRaw_ = false; > + } else { > + qWarning() << "No free buffer available for RAW capture"; > + } > + } We can minimize the amount of code covered by the lock. if (captureRaw_) { FrameBuffer *buffer; { QMutexLocker locker(&mutex_); buffer = !freeBuffers_[rawStream_].isEmpty()) ? freeBuffers_[rawStream_].dequeue() : nullptr; } if (buffer) { request->addBuffer(rawStream_, buffer); captureRaw_ = false; } else { qWarning() << "No free buffer available for RAW capture"; } } Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > + > camera_->queueRequest(request); > } > diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h > index 4856ecc10729159c..295ecc537e9d45bf 100644 > --- a/src/qcam/main_window.h > +++ b/src/qcam/main_window.h > @@ -55,6 +55,8 @@ private Q_SLOTS: > void toggleCapture(bool start); > > void saveImageAs(); > + void captureRaw(); > + void processRaw(FrameBuffer *buffer); > > void queueRequest(FrameBuffer *buffer); > > @@ -75,6 +77,7 @@ private: > QToolBar *toolbar_; > QAction *startStopAction_; > QComboBox *cameraCombo_; > + QAction *saveRaw_; > ViewFinder *viewfinder_; > > QIcon iconPlay_; > @@ -96,6 +99,7 @@ private: > > /* Capture state, buffers queue and statistics */ > bool isCapturing_; > + bool captureRaw_; > Stream *vfStream_; > Stream *rawStream_; > std::map<Stream *, QQueue<FrameBuffer *>> freeBuffers_;
Hi Laurent, Thanks for your feedback. On 2020-05-02 05:38:16 +0300, Laurent Pinchart wrote: > Hi Niklas, > > Thank you for the patch. > > On Sat, May 02, 2020 at 04:10:45AM +0200, Niklas Söderlund wrote: > > Add a toolbar button that captures RAW data to disk. The button is only > > enabled if the camera is configured to provide a raw stream to the > > application. > > > > Only when the capture action is triggered will a request with a raw > > buffer be queued to the camera. > > > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > --- > > * Changes since v2 > > - Use a file dialog > > - Make DNG optional depending on libtiff > > --- > > src/qcam/assets/feathericons/feathericons.qrc | 1 + > > src/qcam/main_window.cpp | 64 ++++++++++++++++++- > > src/qcam/main_window.h | 4 ++ > > 3 files changed, 68 insertions(+), 1 deletion(-) > > > > diff --git a/src/qcam/assets/feathericons/feathericons.qrc b/src/qcam/assets/feathericons/feathericons.qrc > > index c4eb7a0be6884373..fc8213928ece70ea 100644 > > --- a/src/qcam/assets/feathericons/feathericons.qrc > > +++ b/src/qcam/assets/feathericons/feathericons.qrc > > @@ -1,5 +1,6 @@ > > <!DOCTYPE RCC><RCC version="1.0"> > > <qresource> > > +<file>./aperture.svg</file> > > <file>./camera-off.svg</file> > > <file>./play-circle.svg</file> > > <file>./save.svg</file> > > diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp > > index 0bd9f3583ea4f6d4..458da479a9b21b73 100644 > > --- a/src/qcam/main_window.cpp > > +++ b/src/qcam/main_window.cpp > > @@ -27,6 +27,10 @@ > > #include <libcamera/camera_manager.h> > > #include <libcamera/version.h> > > > > +#if HAVE_TIFF > > I wonder if this shouldn't be called HAVE_DNG, as the dependency on > libtiff is internal to the DNGWriter class. I have reworked this to always include dng_writer.h here. And then set HAVE_DNG in dng_writer.h if HAVE_TIFF is set (by meson). > > > +#include "dng_writer.h" > > +#endif > > + > > using namespace libcamera; > > > > /** > > @@ -48,7 +52,8 @@ public: > > }; > > > > MainWindow::MainWindow(CameraManager *cm, const OptionsParser::Options &options) > > - : options_(options), cm_(cm), allocator_(nullptr), isCapturing_(false) > > + : saveRaw_(nullptr), options_(options), cm_(cm), allocator_(nullptr), > > + isCapturing_(false), captureRaw_(false) > > { > > int ret; > > > > @@ -144,6 +149,16 @@ int MainWindow::createToolbars() > > action->setShortcut(QKeySequence::SaveAs); > > connect(action, &QAction::triggered, this, &MainWindow::saveImageAs); > > > > +#if HAVE_TIFF > > + /* Save Raw action. */ > > + action = toolbar_->addAction(QIcon::fromTheme("camera-photo", > > + QIcon(":aperture.svg")), > > + "Save Raw"); > > + action->setEnabled(false); > > + connect(action, &QAction::triggered, this, &MainWindow::captureRaw); > > + saveRaw_ = action; > > +#endif > > I wonder if we should still have the button, but never enable it. If we > don't, users may wonder why the button isn't there. But if we do, they > may wonder why it's never enabled :-) > > Maybe the tooltip text could be set to > > - "RAW capture not available (missing libtiff)" > - "RAW capture not enabled on the command line" > - "Save RAW" > > depending on the situation. Or maybe all of this is overkill :-) I think this is overkill and have kept it as is. My reasoning is that we shall not clutter the GUI with buttons for features the user have opted to not have. Also I think making this code more complex is a bad idea as we already know we need to redisgen the whole UI to support multiple streams properly. > > > + > > return 0; > > } > > > > @@ -369,6 +384,10 @@ int MainWindow::startCapture() > > > > adjustSize(); > > > > + /* Configure the raw capture button. */ > > + if (saveRaw_) > > + saveRaw_->setEnabled(config_->size() == 2); > > + > > /* Allocate and map buffers. */ > > allocator_ = new FrameBufferAllocator(camera_); > > for (StreamConfiguration &config : *config_) { > > @@ -474,6 +493,9 @@ void MainWindow::stopCapture() > > return; > > > > viewfinder_->stop(); > > + if (saveRaw_) > > + saveRaw_->setEnabled(false); > > + captureRaw_ = false; > > > > int ret = camera_->stop(); > > if (ret) > > @@ -524,6 +546,31 @@ void MainWindow::saveImageAs() > > writer.write(image); > > } > > > > +void MainWindow::captureRaw() > > +{ > > + captureRaw_ = true; > > +} > > + > > +void MainWindow::processRaw(FrameBuffer *buffer) > > +{ > > +#if HAVE_TIFF > > + QString defaultPath = QStandardPaths::writableLocation(QStandardPaths::PicturesLocation); > > + QString filename = QFileDialog::getSaveFileName(this, "Save DNG", defaultPath, > > + "DNG Files (*.dng)"); > > + > > + if (filename != "") { > > if (!filename.isEmpty()) { > > > + const MappedBuffer &mapped = mappedBuffers_[buffer]; > > + DNGWriter::write(filename.toStdString().c_str(), camera_.get(), > > + rawStream_->configuration(), buffer, > > + mapped.memory); > > + } > > +#endif > > Blank line here ? > > > + { > > + QMutexLocker locker(&mutex_); > > + freeBuffers_[rawStream_].enqueue(buffer); > > + } > > +} > > + > > /* ----------------------------------------------------------------------------- > > * Request Completion Handling > > */ > > @@ -566,6 +613,9 @@ void MainWindow::processCapture() > > /* Process buffers. */ > > if (buffers.count(vfStream_)) > > processViewfinder(buffers[vfStream_]); > > + > > + if (buffers.count(rawStream_)) > > + processRaw(buffers[rawStream_]); > > } > > > > void MainWindow::processViewfinder(FrameBuffer *buffer) > > @@ -598,5 +648,17 @@ void MainWindow::queueRequest(FrameBuffer *buffer) > > > > request->addBuffer(vfStream_, buffer); > > > > + if (captureRaw_) { > > + QMutexLocker locker(&mutex_); > > + > > + if (!freeBuffers_[rawStream_].isEmpty()) { > > + request->addBuffer(rawStream_, > > + freeBuffers_[rawStream_].dequeue()); > > + captureRaw_ = false; > > + } else { > > + qWarning() << "No free buffer available for RAW capture"; > > + } > > + } > > We can minimize the amount of code covered by the lock. > > if (captureRaw_) { > FrameBuffer *buffer; > > { > QMutexLocker locker(&mutex_); > buffer = !freeBuffers_[rawStream_].isEmpty()) > ? freeBuffers_[rawStream_].dequeue() : nullptr; > } > > if (buffer) { > request->addBuffer(rawStream_, buffer); > captureRaw_ = false; > } else { > qWarning() << "No free buffer available for RAW capture"; > } > } Good idea, thanks. > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > + > > camera_->queueRequest(request); > > } > > diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h > > index 4856ecc10729159c..295ecc537e9d45bf 100644 > > --- a/src/qcam/main_window.h > > +++ b/src/qcam/main_window.h > > @@ -55,6 +55,8 @@ private Q_SLOTS: > > void toggleCapture(bool start); > > > > void saveImageAs(); > > + void captureRaw(); > > + void processRaw(FrameBuffer *buffer); > > > > void queueRequest(FrameBuffer *buffer); > > > > @@ -75,6 +77,7 @@ private: > > QToolBar *toolbar_; > > QAction *startStopAction_; > > QComboBox *cameraCombo_; > > + QAction *saveRaw_; > > ViewFinder *viewfinder_; > > > > QIcon iconPlay_; > > @@ -96,6 +99,7 @@ private: > > > > /* Capture state, buffers queue and statistics */ > > bool isCapturing_; > > + bool captureRaw_; > > Stream *vfStream_; > > Stream *rawStream_; > > std::map<Stream *, QQueue<FrameBuffer *>> freeBuffers_; > > -- > Regards, > > Laurent Pinchart
diff --git a/src/qcam/assets/feathericons/feathericons.qrc b/src/qcam/assets/feathericons/feathericons.qrc index c4eb7a0be6884373..fc8213928ece70ea 100644 --- a/src/qcam/assets/feathericons/feathericons.qrc +++ b/src/qcam/assets/feathericons/feathericons.qrc @@ -1,5 +1,6 @@ <!DOCTYPE RCC><RCC version="1.0"> <qresource> +<file>./aperture.svg</file> <file>./camera-off.svg</file> <file>./play-circle.svg</file> <file>./save.svg</file> diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp index 0bd9f3583ea4f6d4..458da479a9b21b73 100644 --- a/src/qcam/main_window.cpp +++ b/src/qcam/main_window.cpp @@ -27,6 +27,10 @@ #include <libcamera/camera_manager.h> #include <libcamera/version.h> +#if HAVE_TIFF +#include "dng_writer.h" +#endif + using namespace libcamera; /** @@ -48,7 +52,8 @@ public: }; MainWindow::MainWindow(CameraManager *cm, const OptionsParser::Options &options) - : options_(options), cm_(cm), allocator_(nullptr), isCapturing_(false) + : saveRaw_(nullptr), options_(options), cm_(cm), allocator_(nullptr), + isCapturing_(false), captureRaw_(false) { int ret; @@ -144,6 +149,16 @@ int MainWindow::createToolbars() action->setShortcut(QKeySequence::SaveAs); connect(action, &QAction::triggered, this, &MainWindow::saveImageAs); +#if HAVE_TIFF + /* Save Raw action. */ + action = toolbar_->addAction(QIcon::fromTheme("camera-photo", + QIcon(":aperture.svg")), + "Save Raw"); + action->setEnabled(false); + connect(action, &QAction::triggered, this, &MainWindow::captureRaw); + saveRaw_ = action; +#endif + return 0; } @@ -369,6 +384,10 @@ int MainWindow::startCapture() adjustSize(); + /* Configure the raw capture button. */ + if (saveRaw_) + saveRaw_->setEnabled(config_->size() == 2); + /* Allocate and map buffers. */ allocator_ = new FrameBufferAllocator(camera_); for (StreamConfiguration &config : *config_) { @@ -474,6 +493,9 @@ void MainWindow::stopCapture() return; viewfinder_->stop(); + if (saveRaw_) + saveRaw_->setEnabled(false); + captureRaw_ = false; int ret = camera_->stop(); if (ret) @@ -524,6 +546,31 @@ void MainWindow::saveImageAs() writer.write(image); } +void MainWindow::captureRaw() +{ + captureRaw_ = true; +} + +void MainWindow::processRaw(FrameBuffer *buffer) +{ +#if HAVE_TIFF + QString defaultPath = QStandardPaths::writableLocation(QStandardPaths::PicturesLocation); + QString filename = QFileDialog::getSaveFileName(this, "Save DNG", defaultPath, + "DNG Files (*.dng)"); + + if (filename != "") { + const MappedBuffer &mapped = mappedBuffers_[buffer]; + DNGWriter::write(filename.toStdString().c_str(), camera_.get(), + rawStream_->configuration(), buffer, + mapped.memory); + } +#endif + { + QMutexLocker locker(&mutex_); + freeBuffers_[rawStream_].enqueue(buffer); + } +} + /* ----------------------------------------------------------------------------- * Request Completion Handling */ @@ -566,6 +613,9 @@ void MainWindow::processCapture() /* Process buffers. */ if (buffers.count(vfStream_)) processViewfinder(buffers[vfStream_]); + + if (buffers.count(rawStream_)) + processRaw(buffers[rawStream_]); } void MainWindow::processViewfinder(FrameBuffer *buffer) @@ -598,5 +648,17 @@ void MainWindow::queueRequest(FrameBuffer *buffer) request->addBuffer(vfStream_, buffer); + if (captureRaw_) { + QMutexLocker locker(&mutex_); + + if (!freeBuffers_[rawStream_].isEmpty()) { + request->addBuffer(rawStream_, + freeBuffers_[rawStream_].dequeue()); + captureRaw_ = false; + } else { + qWarning() << "No free buffer available for RAW capture"; + } + } + camera_->queueRequest(request); } diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h index 4856ecc10729159c..295ecc537e9d45bf 100644 --- a/src/qcam/main_window.h +++ b/src/qcam/main_window.h @@ -55,6 +55,8 @@ private Q_SLOTS: void toggleCapture(bool start); void saveImageAs(); + void captureRaw(); + void processRaw(FrameBuffer *buffer); void queueRequest(FrameBuffer *buffer); @@ -75,6 +77,7 @@ private: QToolBar *toolbar_; QAction *startStopAction_; QComboBox *cameraCombo_; + QAction *saveRaw_; ViewFinder *viewfinder_; QIcon iconPlay_; @@ -96,6 +99,7 @@ private: /* Capture state, buffers queue and statistics */ bool isCapturing_; + bool captureRaw_; Stream *vfStream_; Stream *rawStream_; std::map<Stream *, QQueue<FrameBuffer *>> freeBuffers_;
Add a toolbar button that captures RAW data to disk. The button is only enabled if the camera is configured to provide a raw stream to the application. Only when the capture action is triggered will a request with a raw buffer be queued to the camera. Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> --- * Changes since v2 - Use a file dialog - Make DNG optional depending on libtiff --- src/qcam/assets/feathericons/feathericons.qrc | 1 + src/qcam/main_window.cpp | 64 ++++++++++++++++++- src/qcam/main_window.h | 4 ++ 3 files changed, 68 insertions(+), 1 deletion(-)