[Merge] lp:~phablet-team/qtubuntu-camera/fix-exif-timestamp into lp:qtubuntu-camera

[Jim Hodapp]

Review: Needs Fixing code

Looks pretty good, have a few suggestions inline. I'd like to see a unit test for the function updateJpegMetadata() as well.

Diff comments:

> === modified file 'debian/control'
> --- debian/control	2014-08-05 20:10:08 +0000
> +++ debian/control	2014-09-23 18:15:50 +0000
> @@ -13,7 +13,8 @@
>                 pkg-config,
>                 qt5-default,
>                 qtmultimedia5-dev,
> -               libpulse-dev
> +               libpulse-dev,
> +               libexiv2-dev
>  Standards-Version: 3.9.4
>  Homepage: https://launchpad.net/qtubuntu-camera
>  # If you aren't a member of ~phablet-team but need to upload packaging changes,
> 
> === modified file 'src/aalimagecapturecontrol.cpp'
> --- src/aalimagecapturecontrol.cpp	2014-09-18 14:25:53 +0000
> +++ src/aalimagecapturecontrol.cpp	2014-09-23 18:15:50 +0000
> @@ -23,13 +23,13 @@
>  
>  #include <hybris/camera/camera_compatibility_layer.h>
>  #include <hybris/camera/camera_compatibility_layer_capabilities.h>
> +#include <exiv2/exiv2.hpp>
>  
>  #include <QDir>
>  #include <QFile>
>  #include <QFileInfo>
>  #include <QMediaPlayer>
>  #include <QStandardPaths>
> -#include <QTemporaryFile>
>  #include <QDateTime>
>  
>  #include <cmath>
> @@ -264,28 +264,74 @@
>      }
>  }
>  
> +bool AalImageCaptureControl::updateJpegMetadata(void* data, uint32_t dataSize, QTemporaryFile* destination)
> +{

Add a check to make sure that data and destination are not == nullptr

> +    Exiv2::Image::AutoPtr image;
> +    try {
> +        image = Exiv2::ImageFactory::open((Exiv2::byte*) data, dataSize);

Please use a C++ cast, i.e. static_cast<Exiv2::byte*>

> +        if (!image.get()) {
> +            return false;
> +        }
> +    } catch(const Exiv2::AnyError&) {
> +        return false;
> +    }
> +
> +    try {
> +        image->readMetadata();
> +        Exiv2::ExifData ed = image->exifData();
> +        QString now = QDateTime::currentDateTime().toString("yyyy:MM:dd HH:mm:ss");

const QString now

> +        ed["Exif.Photo.DateTimeOriginal"].setValue(now.toStdString());
> +        ed["Exif.Photo.DateTimeDigitized"].setValue(now.toStdString());
> +        image->setExifData(ed);
> +        image->writeMetadata();
> +    } catch(const Exiv2::AnyError&) {
> +        return false;
> +    }
> +
> +    if (!destination->open()) {
> +        return false;
> +    }
> +
> +    try {
> +        Exiv2::BasicIo& io = image->io();
> +        char* modifiedMetadata = (char*) io.mmap();

Please use a C++ cast, i.e. static_cast<char*>

Also, match the style of the rest of the code by using const char *modifiedMetadata

> +        long size = io.size();

const long size = io.size();

> +        qint64 writtenSize = destination->write(modifiedMetadata, size);

const qint64 writtenSize

> +        //unmap
> +        destination->close();
> +        return (writtenSize == size);
> +
> +    } catch(const Exiv2::AnyError&) {
> +        destination->close();
> +        return false;
> +    }
> +}
> +
>  void AalImageCaptureControl::saveJpeg(void *data, uint32_t dataSize)
>  {
>      if (m_pendingCaptureFile.isNull() || !m_service->androidControl())
>          return;
>  
>      QTemporaryFile file;
> -    if (!file.open()) {
> -        emit error(m_lastRequestId, QCameraImageCapture::ResourceError,
> -                   QString("Could not open temprary file %1").arg(file.fileName()));
> -        m_pendingCaptureFile.clear();
> -        m_service->updateCaptureReady();
> -        return;
> -    }
> +    if (!updateJpegMetadata(data, dataSize, &file)) {
> +        qDebug() << "Failed to update EXIF timestamps. Picture will be saved as UTC timezone.";

Use qWarning instead of qDebug

> +        if (!file.open()) {
> +            emit error(m_lastRequestId, QCameraImageCapture::ResourceError,
> +                       QString("Could not open temprary file %1").arg(file.fileName()));
> +            m_pendingCaptureFile.clear();
> +            m_service->updateCaptureReady();
> +            return;
> +        }
>  
> -    qint64 writtenSize = file.write((const char*)data, dataSize);
> -    file.close();
> -    if (writtenSize != dataSize) {
> -        emit error(m_lastRequestId, QCameraImageCapture::ResourceError,
> -                   QString("Could not write file %1").arg(file.fileName()));
> -        m_pendingCaptureFile.clear();
> -        m_service->updateCaptureReady();
> -        return;
> +        qint64 writtenSize = file.write((const char*)data, dataSize);

const qint64 writtenSize

Please use a C++ cast, i.e. static_cast<const char*>

> +        file.close();
> +        if (writtenSize != dataSize) {
> +            emit error(m_lastRequestId, QCameraImageCapture::ResourceError,
> +                       QString("Could not write file %1").arg(file.fileName()));
> +            m_pendingCaptureFile.clear();
> +            m_service->updateCaptureReady();
> +            return;
> +        }
>      }
>  
>      QFile finalFile(file.fileName());
> 
> === modified file 'src/aalimagecapturecontrol.h'
> --- src/aalimagecapturecontrol.h	2013-09-17 09:11:36 +0000
> +++ src/aalimagecapturecontrol.h	2014-09-23 18:15:50 +0000
> @@ -19,6 +19,7 @@
>  
>  #include <QCameraImageCaptureControl>
>  #include <QString>
> +#include <QTemporaryFile>
>  #include <storagemanager.h>
>  
>  #include <stdint.h>
> @@ -67,6 +68,7 @@
>      float getScreenAspectRatio();
>      void getPriorityAspectRatios();
>      void saveJpeg(void* data, uint32_t dataSize);
> +    bool updateJpegMetadata(void* data, uint32_t dataSize, QTemporaryFile* destination);
>  
>      AalCameraService *m_service;
>      AalCameraControl *m_cameraControl;
> 
> === modified file 'src/src.pro'
> --- src/src.pro	2014-07-30 19:35:51 +0000
> +++ src/src.pro	2014-09-23 18:15:50 +0000
> @@ -18,6 +18,9 @@
>      -lpulse \
>      -lpulse-simple
>  
> +CONFIG += link_pkgconfig
> +PKGCONFIG += exiv2
> +
>  OTHER_FILES += aalcamera.json
>  
>  HEADERS += \
> 
> === modified file 'unittests/aalimagecapturecontrol/aalimagecapturecontrol.pro'
> --- unittests/aalimagecapturecontrol/aalimagecapturecontrol.pro	2014-01-16 14:20:08 +0000
> +++ unittests/aalimagecapturecontrol/aalimagecapturecontrol.pro	2014-09-23 18:15:50 +0000
> @@ -8,6 +8,9 @@
>  INCLUDEPATH += ../../src
>  INCLUDEPATH += ../mocks/aal
>  
> +CONFIG += link_pkgconfig
> +PKGCONFIG += exiv2
> +
>  HEADERS += ../../src/aalimagecapturecontrol.h \
>      ../../src/aalcameraservice.h \
>      ../../src/aalimageencodercontrol.h \
> 


-- 
https://code.launchpad.net/~phablet-team/qtubuntu-camera/fix-exif-timestamp/+merge/235691
Your team Ubuntu Phablet Team is subscribed to branch lp:qtubuntu-camera.