Review Request 113397: RAW preview in gwenview

Thu Nov 7 15:21:19 UTC 2013

> On Nov. 7, 2013, 9:01 a.m., Aurélien Gâteau wrote:
> > Great news regarding kdcraw getting the necessary changes, it's nice to keep the changes to the minimum.
> > 
> > Regarding the loading of half previews: next time, avoid adding new features in the middle of the review. Even if the feature is a good idea (it is!) if you keep adding code that thing is never going to land.

Thanks for the process clarification. It's never gonna happen again here. Now, I assume the feature to be complete. I have only one question: for the half previewed raws, what do you think is better to be shown as a image resolution on the thumbnail page?

1) the real raw resolution (as it is in the code now, the size is being doubled if it was loaded via loadHalfPreview)
2) the preview's resolution (halved)

I implemented the former, but if the raw has embedded preview of reduced resolution (such as Sony of Fuji), then there's the preview's resolution anyways. Digging the real image size from the raw metadata seems as an overkill to me. Now, the user can be confused that the displayed image is smaller than what was displayed in the thumbnail page.

> On Nov. 7, 2013, 9:01 a.m., Aurélien Gâteau wrote:
> > lib/document/loadingdocumentimpl.cpp, line 253
> > <http://git.reviewboard.kde.org/r/113397/diff/7/?file=210255#file210255line253>
> >
> >     I am a bit worried about this: what is the value of mFormat in the case of previewing a raw file?

It should be outside of the block (just as it was before). Fixed.

mFormat is set to mFormatHint (ie. the file's extension) at the end of the raw support code block. It's poor, but using reader.format() gives TIFF for raws which is not good.

> On Nov. 7, 2013, 9:01 a.m., Aurélien Gâteau wrote:
> > lib/thumbnailprovider/thumbnailgenerator.cpp, line 96
> > <http://git.reviewboard.kde.org/r/113397/diff/7/?file=210258#file210258line96>
> >
> >     Reading this again, I don't understand the comment: why do you consider loading the image from data a dirty hack?

That comment was a leftover from preview development, where I got ugly looking stuff there. The code has been restructured so it is not valid anymore.

> On Nov. 7, 2013, 9:01 a.m., Aurélien Gâteau wrote:
> > lib/thumbnailprovider/thumbnailgenerator.cpp, line 133
> > <http://git.reviewboard.kde.org/r/113397/diff/7/?file=210258#file210258line133>
> >
> >     I don't have an opinion regarding raw files, but in the case of jpeg, I really want the code to use the embedded thumbnail if it is available.

Oh, IC.. I misunderstood that problem. Thanks for the catch and clarification. I dropped that part of code by mistake. Actually, we get jpeg out of the raw files (either way). It's reasonable to try fetching embedded thumbnail from the raw preview, too.

- Martin

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/113397/#review43199
-----------------------------------------------------------

On Nov. 7, 2013, 3:03 p.m., Martin Kyral wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/113397/
> -----------------------------------------------------------
> 
> (Updated Nov. 7, 2013, 3:03 p.m.)
> 
> 
> Review request for Gwenview and Aurélien Gâteau.
> 
> 
> Repository: gwenview
> 
> 
> Description
> -------
> 
> This is a review of patch enabling raw support in gwenview. It uses KDcraw to extract the embedded jpeg preview (most of the raw formats contain it for the purposes of quick viewing the photos on the camera display) so it is fast, implementing was quite easy and theoretically it enables gwenview to support everything dcraw supports (practically we need to test more formats). The patch does not perform demosaicing and I don't think it shall - digikam, darktable or rawtherapee are much more suited for developing the 'digital negatives'.
> 
> The patch fixes BZ#158788
> 
> The patch enables support for Nikon NEF and NRW formats, Canon's CR2, Pentax PEF, Sony ARW and Adobe DNG. If you want to try another format, just adding it's MIME type to the list in lib/mimetypeutils.cpp shall be enough.
> 
> The patch as is contains just the basic functionality (generating proper thumbnails and viewing the raw images incl. EXIF metadata), but it is well usable. However, there is still a lot of stuff to do (see the commit message in the patch) so I'd appreciate any help.
> 
> 
> Diffs
> -----
> 
>   CMakeLists.txt 4dafb7e 
>   lib/CMakeLists.txt c6ffe14 
>   lib/document/loadingdocumentimpl.cpp fbad8ff 
>   lib/gwenviewconfig.kcfg 231fd5e 
>   lib/mimetypeutils.cpp 546346f 
>   lib/thumbnailprovider/thumbnailgenerator.cpp 8b98904 
> 
> Diff: http://git.reviewboard.kde.org/r/113397/diff/
> 
> 
> Testing
> -------
> 
> Tested on some sample raw files taken from the camera reviews on http://www.photographyblog.com/ and bunch of my own photos. Nikon (tried D50, D70s, D90, D800 and Coolpix P7000), Canon (tried 7D, 5D Mark3 and 6D) and Pentax (K-X, K30) have the preview full-res, while Sony raw files (A900, A850, A58, Nex 3, Nex 6) have only 1616x1050.
> 
> 
> Thanks,
> 
> Martin Kyral
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/gwenview-devel/attachments/20131107/cf58152b/attachment-0001.html>