| Message ID | 20200824204646.16866-1-email@uajain.com |
|---|---|
| Headers | show |
| Series |
|
| Related | show |
Hi Umang, Thanks for taking this on. On 24/08/2020 21:46, Umang Jain wrote: > Hi all, > > This patch series targets adding initial EXIF metadata support > for the frames captured in Android/Libcamera framework. > > It is initial in the sense, it adds the base infrastructure for > Exif metadata support via 'libexif' and also encodes a initial > set of metadata tags into the frames. > > Testing: > The testing of this work was carried out via vivid pipeline-handler > in conjunction with patch in `cam` using EncoderLibJpeg as the > file sink [1]. That way we could see what EXIF metadata could be written > and inspect that `exif` or `exiv2` commandline tools. > > ($) ./build/src/cam/cam -c vivid -C5 -F -s pixelformat=NV12 > > Sample output (1): exif frame-stream0-000000.jpg > EXIF tags in 'frame-stream0-000000.jpg' ('Intel' byte order): > --------------------+---------------------------------------------------------- > Tag |Value > --------------------+---------------------------------------------------------- > Image Width |1280 > Image Length |720 > Manufacturer |Libcamera Looking at my branch, it was of course me who put this in (incorrectly) with a capital L, but please make it lowercase ;-) > Model |Camera2111 I assume you set this Camera2111 string manually for testing ? > Orientation |Bottom-left > X-Resolution |72 > Y-Resolution |72 > Resolution Unit |Inch > Date and Time |Sun Oct 13 07:03:20 734137 > > YCbCr Positioning |Centered > Exif Version |Exif Version 2.1 > Date and Time (Origi|Sun Oct 13 07:03:20 734137 > > Date and Time (Digit|Sun Oct 13 07:03:20 734137 Hrm ... we might have some work to do on converting the timestamp values appropriately here. That doesn't look right at all ;-) > > Components Configura|Y Cb Cr - > FlashPixVersion |FlashPix Version 1.0 > Color Space |Internal error (unknown value 65535) This one needs to be checked if there is an error/incorrect parameter set. > Pixel X Dimension |1280 > Pixel Y Dimension |720 > > > > Sample output (2): exiv2 frame-stream0-000000.jpg > File name : frame-stream0-000000.jpg > File size : 38787 Bytes > MIME type : image/jpeg > Image size : 1280 x 720 > Camera make : Libcamera > Camera model : Camera2111 > Image timestamp : Sun Oct 13 07:03:20 734137 > > Image number : Hrm, I wonder if we should put the sequence number in here. > Exposure time : > Aperture : > Exposure bias : > Flash : > Flash bias : > Focal length : > Subject distance: > ISO speed : > Exposure mode : > Metering mode : > Macro mode : > Image quality : Plenty of parameters we'll want to get from the IPAs there, but that will be later. > Exif Resolution : 1280 x 720 > White balance : > Thumbnail : None > Copyright : > Exif comment : > > There are still a lot of tags that needs to be populated, > that can be built on top of this groundwork. > > [1]: https://github.com/kbingham/libcamera/commit/10ed8ec > > Kieran Bingham (1): > libcamera: android: Add EXIF infrastructure > > Umang Jain (2): > android: jpeg: Pass a Exif object while encoding > android: Support initial set of EXIF metadata tags > > src/android/camera_device.cpp | 12 +- > src/android/jpeg/encoder.h | 5 +- > src/android/jpeg/encoder_libjpeg.cpp | 15 +- > src/android/jpeg/encoder_libjpeg.h | 3 +- > src/android/jpeg/exif.cpp | 210 +++++++++++++++++++++++++++ > src/android/jpeg/exif.h | 53 +++++++ > src/android/meson.build | 2 + > 7 files changed, 296 insertions(+), 4 deletions(-) > create mode 100644 src/android/jpeg/exif.cpp > create mode 100644 src/android/jpeg/exif.h >
Hi Kieran, Thanks for the review. Almost all of the comments from this and other patches review should be addressed now in v2. On 8/25/20 2:58 AM, Kieran Bingham wrote: > Hi Umang, > > Thanks for taking this on. > > On 24/08/2020 21:46, Umang Jain wrote: >> Hi all, >> >> This patch series targets adding initial EXIF metadata support >> for the frames captured in Android/Libcamera framework. >> >> It is initial in the sense, it adds the base infrastructure for >> Exif metadata support via 'libexif' and also encodes a initial >> set of metadata tags into the frames. >> >> Testing: >> The testing of this work was carried out via vivid pipeline-handler >> in conjunction with patch in `cam` using EncoderLibJpeg as the >> file sink [1]. That way we could see what EXIF metadata could be written >> and inspect that `exif` or `exiv2` commandline tools. >> >> ($) ./build/src/cam/cam -c vivid -C5 -F -s pixelformat=NV12 >> >> Sample output (1): exif frame-stream0-000000.jpg >> EXIF tags in 'frame-stream0-000000.jpg' ('Intel' byte order): >> --------------------+---------------------------------------------------------- >> Tag |Value >> --------------------+---------------------------------------------------------- >> Image Width |1280 >> Image Length |720 >> Manufacturer |Libcamera > Looking at my branch, it was of course me who put this in (incorrectly) > with a capital L, but please make it lowercase ;-) > >> Model |Camera2111 > I assume you set this Camera2111 string manually for testing ? Yes, hardcoded in cam's frame_sink_compressor.cpp > > >> Orientation |Bottom-left >> X-Resolution |72 >> Y-Resolution |72 >> Resolution Unit |Inch >> Date and Time |Sun Oct 13 07:03:20 734137 >> >> YCbCr Positioning |Centered >> Exif Version |Exif Version 2.1 >> Date and Time (Origi|Sun Oct 13 07:03:20 734137 >> >> Date and Time (Digit|Sun Oct 13 07:03:20 734137 > Hrm ... we might have some work to do on converting the timestamp values > appropriately here. That doesn't look right at all ;-) We now use, 'time of encode' as the frame's timestamp as discussed over IRC. > > >> Components Configura|Y Cb Cr - >> FlashPixVersion |FlashPix Version 1.0 >> Color Space |Internal error (unknown value 65535) > This one needs to be checked if there is an error/incorrect parameter set. This one still needs looking. I will queue it up. > > >> Pixel X Dimension |1280 >> Pixel Y Dimension |720 >> >> >> >> Sample output (2): exiv2 frame-stream0-000000.jpg >> File name : frame-stream0-000000.jpg >> File size : 38787 Bytes >> MIME type : image/jpeg >> Image size : 1280 x 720 >> Camera make : Libcamera >> Camera model : Camera2111 >> Image timestamp : Sun Oct 13 07:03:20 734137 >> >> Image number : > Hrm, I wonder if we should put the sequence number in here. No idea, seems sane though. :) > > >> Exposure time : >> Aperture : >> Exposure bias : >> Flash : >> Flash bias : >> Focal length : >> Subject distance: >> ISO speed : >> Exposure mode : >> Metering mode : >> Macro mode : >> Image quality : > Plenty of parameters we'll want to get from the IPAs there, but that > will be later. Indeed, I intentionally attached exiv2's output to see where we stand and what should be the so called 'next fields' to populate. We can build it on top of this > >> Exif Resolution : 1280 x 720 >> White balance : >> Thumbnail : None >> Copyright : >> Exif comment : >> >> There are still a lot of tags that needs to be populated, >> that can be built on top of this groundwork. >> >> [1]: https://github.com/kbingham/libcamera/commit/10ed8ec >> >> Kieran Bingham (1): >> libcamera: android: Add EXIF infrastructure >> >> Umang Jain (2): >> android: jpeg: Pass a Exif object while encoding >> android: Support initial set of EXIF metadata tags >> >> src/android/camera_device.cpp | 12 +- >> src/android/jpeg/encoder.h | 5 +- >> src/android/jpeg/encoder_libjpeg.cpp | 15 +- >> src/android/jpeg/encoder_libjpeg.h | 3 +- >> src/android/jpeg/exif.cpp | 210 +++++++++++++++++++++++++++ >> src/android/jpeg/exif.h | 53 +++++++ >> src/android/meson.build | 2 + >> 7 files changed, 296 insertions(+), 4 deletions(-) >> create mode 100644 src/android/jpeg/exif.cpp >> create mode 100644 src/android/jpeg/exif.h >>
Hi Umang, On Tue, Aug 25, 2020 at 08:16:44PM +0000, Umang Jain wrote: > Hi Kieran, > > Thanks for the review. Almost all of the comments from this and other > patches review should be addressed now in v2. > > On 8/25/20 2:58 AM, Kieran Bingham wrote: > > Hi Umang, > > > > Thanks for taking this on. > > > > On 24/08/2020 21:46, Umang Jain wrote: > >> Hi all, > >> > >> This patch series targets adding initial EXIF metadata support > >> for the frames captured in Android/Libcamera framework. > >> > >> It is initial in the sense, it adds the base infrastructure for > >> Exif metadata support via 'libexif' and also encodes a initial > >> set of metadata tags into the frames. > >> > >> Testing: > >> The testing of this work was carried out via vivid pipeline-handler > >> in conjunction with patch in `cam` using EncoderLibJpeg as the > >> file sink [1]. That way we could see what EXIF metadata could be written > >> and inspect that `exif` or `exiv2` commandline tools. > >> > >> ($) ./build/src/cam/cam -c vivid -C5 -F -s pixelformat=NV12 > >> > >> Sample output (1): exif frame-stream0-000000.jpg > >> EXIF tags in 'frame-stream0-000000.jpg' ('Intel' byte order): > >> --------------------+---------------------------------------------------------- > >> Tag |Value > >> --------------------+---------------------------------------------------------- > >> Image Width |1280 > >> Image Length |720 > >> Manufacturer |Libcamera > > > > Looking at my branch, it was of course me who put this in (incorrectly) > > with a capital L, but please make it lowercase ;-) > > > >> Model |Camera2111 > > > > I assume you set this Camera2111 string manually for testing ? > > Yes, hardcoded in cam's frame_sink_compressor.cpp > > >> Orientation |Bottom-left > >> X-Resolution |72 > >> Y-Resolution |72 > >> Resolution Unit |Inch > >> Date and Time |Sun Oct 13 07:03:20 734137 > >> > >> YCbCr Positioning |Centered > >> Exif Version |Exif Version 2.1 > >> Date and Time (Origi|Sun Oct 13 07:03:20 734137 > >> > >> Date and Time (Digit|Sun Oct 13 07:03:20 734137 > > > > Hrm ... we might have some work to do on converting the timestamp values > > appropriately here. That doesn't look right at all ;-) > > We now use, 'time of encode' as the frame's timestamp as discussed over IRC. > > >> Components Configura|Y Cb Cr - > >> FlashPixVersion |FlashPix Version 1.0 > >> Color Space |Internal error (unknown value 65535) > > > > This one needs to be checked if there is an error/incorrect parameter set. > > This one still needs looking. I will queue it up. > > >> Pixel X Dimension |1280 > >> Pixel Y Dimension |720 > >> > >> > >> > >> Sample output (2): exiv2 frame-stream0-000000.jpg > >> File name : frame-stream0-000000.jpg > >> File size : 38787 Bytes > >> MIME type : image/jpeg > >> Image size : 1280 x 720 > >> Camera make : Libcamera > >> Camera model : Camera2111 > >> Image timestamp : Sun Oct 13 07:03:20 734137 > >> Image number : > > > > Hrm, I wonder if we should put the sequence number in here. > > No idea, seems sane though. :) That doesn't seem right to me. What is this tag ? I've grepped "Image number" in the exiv2 sources, and the only occurrences are related to tags specific to Minolta and Canon. > >> Exposure time : > >> Aperture : > >> Exposure bias : > >> Flash : > >> Flash bias : > >> Focal length : > >> Subject distance: > >> ISO speed : > >> Exposure mode : > >> Metering mode : > >> Macro mode : > >> Image quality : > > > > Plenty of parameters we'll want to get from the IPAs there, but that > > will be later. > > Indeed, I intentionally attached exiv2's output to see where we stand > and what should be the so called 'next fields' to populate. > > We can build it on top of this > > >> Exif Resolution : 1280 x 720 > >> White balance : > >> Thumbnail : None > >> Copyright : > >> Exif comment : > >> > >> There are still a lot of tags that needs to be populated, > >> that can be built on top of this groundwork. > >> > >> [1]: https://github.com/kbingham/libcamera/commit/10ed8ec > >> > >> Kieran Bingham (1): > >> libcamera: android: Add EXIF infrastructure > >> > >> Umang Jain (2): > >> android: jpeg: Pass a Exif object while encoding > >> android: Support initial set of EXIF metadata tags > >> > >> src/android/camera_device.cpp | 12 +- > >> src/android/jpeg/encoder.h | 5 +- > >> src/android/jpeg/encoder_libjpeg.cpp | 15 +- > >> src/android/jpeg/encoder_libjpeg.h | 3 +- > >> src/android/jpeg/exif.cpp | 210 +++++++++++++++++++++++++++ > >> src/android/jpeg/exif.h | 53 +++++++ > >> src/android/meson.build | 2 + > >> 7 files changed, 296 insertions(+), 4 deletions(-) > >> create mode 100644 src/android/jpeg/exif.cpp > >> create mode 100644 src/android/jpeg/exif.h
H Kieran, On 8/25/20 2:58 AM, Kieran Bingham wrote: > Hi Umang, > > Thanks for taking this on. > > On 24/08/2020 21:46, Umang Jain wrote: >> Hi all, >> >> This patch series targets adding initial EXIF metadata support >> for the frames captured in Android/Libcamera framework. >> >> It is initial in the sense, it adds the base infrastructure for >> Exif metadata support via 'libexif' and also encodes a initial >> set of metadata tags into the frames. >> >> Testing: >> The testing of this work was carried out via vivid pipeline-handler >> in conjunction with patch in `cam` using EncoderLibJpeg as the >> file sink [1]. That way we could see what EXIF metadata could be written >> and inspect that `exif` or `exiv2` commandline tools. >> >> ($) ./build/src/cam/cam -c vivid -C5 -F -s pixelformat=NV12 >> >> Sample output (1): exif frame-stream0-000000.jpg >> EXIF tags in 'frame-stream0-000000.jpg' ('Intel' byte order): >> --------------------+---------------------------------------------------------- >> Tag |Value >> --------------------+---------------------------------------------------------- >> Image Width |1280 >> Image Length |720 >> Manufacturer |Libcamera > Looking at my branch, it was of course me who put this in (incorrectly) > with a capital L, but please make it lowercase ;-) > >> Model |Camera2111 > I assume you set this Camera2111 string manually for testing ? > > >> Orientation |Bottom-left >> X-Resolution |72 >> Y-Resolution |72 >> Resolution Unit |Inch >> Date and Time |Sun Oct 13 07:03:20 734137 >> >> YCbCr Positioning |Centered >> Exif Version |Exif Version 2.1 >> Date and Time (Origi|Sun Oct 13 07:03:20 734137 >> >> Date and Time (Digit|Sun Oct 13 07:03:20 734137 > Hrm ... we might have some work to do on converting the timestamp values > appropriately here. That doesn't look right at all ;-) > > >> Components Configura|Y Cb Cr - >> FlashPixVersion |FlashPix Version 1.0 >> Color Space |Internal error (unknown value 65535) > This one needs to be checked if there is an error/incorrect parameter set. I found out that, this is not an error. The error messsage however, is misleading. https://github.com/Aries85/LightZone/issues/171#issuecomment-195730879 Color Space 65535 => Uncalibrated. We are running exif_data_fix(data_); in exif's constructor, so I think it's where this "Uncalibrated" value is getting set as the default value for color space. Can we query colorspace as of now? Then we can try setting it here as well. > >> Pixel X Dimension |1280 >> Pixel Y Dimension |720 >> >> >> >> Sample output (2): exiv2 frame-stream0-000000.jpg >> File name : frame-stream0-000000.jpg >> File size : 38787 Bytes >> MIME type : image/jpeg >> Image size : 1280 x 720 >> Camera make : Libcamera >> Camera model : Camera2111 >> Image timestamp : Sun Oct 13 07:03:20 734137 >> >> Image number : > Hrm, I wonder if we should put the sequence number in here. > > >> Exposure time : >> Aperture : >> Exposure bias : >> Flash : >> Flash bias : >> Focal length : >> Subject distance: >> ISO speed : >> Exposure mode : >> Metering mode : >> Macro mode : >> Image quality : > Plenty of parameters we'll want to get from the IPAs there, but that > will be later. > >> Exif Resolution : 1280 x 720 >> White balance : >> Thumbnail : None >> Copyright : >> Exif comment : >> >> There are still a lot of tags that needs to be populated, >> that can be built on top of this groundwork. >> >> [1]: https://github.com/kbingham/libcamera/commit/10ed8ec >> >> Kieran Bingham (1): >> libcamera: android: Add EXIF infrastructure >> >> Umang Jain (2): >> android: jpeg: Pass a Exif object while encoding >> android: Support initial set of EXIF metadata tags >> >> src/android/camera_device.cpp | 12 +- >> src/android/jpeg/encoder.h | 5 +- >> src/android/jpeg/encoder_libjpeg.cpp | 15 +- >> src/android/jpeg/encoder_libjpeg.h | 3 +- >> src/android/jpeg/exif.cpp | 210 +++++++++++++++++++++++++++ >> src/android/jpeg/exif.h | 53 +++++++ >> src/android/meson.build | 2 + >> 7 files changed, 296 insertions(+), 4 deletions(-) >> create mode 100644 src/android/jpeg/exif.cpp >> create mode 100644 src/android/jpeg/exif.h >>
Hi Umang, On Thu, Aug 27, 2020 at 05:53:54AM +0000, Umang Jain wrote: > On 8/25/20 2:58 AM, Kieran Bingham wrote: > > On 24/08/2020 21:46, Umang Jain wrote: > >> Hi all, > >> > >> This patch series targets adding initial EXIF metadata support > >> for the frames captured in Android/Libcamera framework. > >> > >> It is initial in the sense, it adds the base infrastructure for > >> Exif metadata support via 'libexif' and also encodes a initial > >> set of metadata tags into the frames. > >> > >> Testing: > >> The testing of this work was carried out via vivid pipeline-handler > >> in conjunction with patch in `cam` using EncoderLibJpeg as the > >> file sink [1]. That way we could see what EXIF metadata could be written > >> and inspect that `exif` or `exiv2` commandline tools. > >> > >> ($) ./build/src/cam/cam -c vivid -C5 -F -s pixelformat=NV12 > >> > >> Sample output (1): exif frame-stream0-000000.jpg > >> EXIF tags in 'frame-stream0-000000.jpg' ('Intel' byte order): > >> --------------------+---------------------------------------------------------- > >> Tag |Value > >> --------------------+---------------------------------------------------------- > >> Image Width |1280 > >> Image Length |720 > >> Manufacturer |Libcamera > > > > Looking at my branch, it was of course me who put this in (incorrectly) > > with a capital L, but please make it lowercase ;-) > > > >> Model |Camera2111 > > > > I assume you set this Camera2111 string manually for testing ? > > > >> Orientation |Bottom-left > >> X-Resolution |72 > >> Y-Resolution |72 > >> Resolution Unit |Inch > >> Date and Time |Sun Oct 13 07:03:20 734137 > >> > >> YCbCr Positioning |Centered > >> Exif Version |Exif Version 2.1 > >> Date and Time (Origi|Sun Oct 13 07:03:20 734137 > >> > >> Date and Time (Digit|Sun Oct 13 07:03:20 734137 > > > > Hrm ... we might have some work to do on converting the timestamp values > > appropriately here. That doesn't look right at all ;-) > > > >> Components Configura|Y Cb Cr - > >> FlashPixVersion |FlashPix Version 1.0 > >> Color Space |Internal error (unknown value 65535) > > > This one needs to be checked if there is an error/incorrect parameter set. > I found out that, this is not an error. The error messsage however, is > misleading. > https://github.com/Aries85/LightZone/issues/171#issuecomment-195730879 > > Color Space 65535 => Uncalibrated. > We are running exif_data_fix(data_); in exif's constructor, so I think it's where > this "Uncalibrated" value is getting set as the default value for color space. > > Can we query colorspace as of now? Then we can try setting it here as well. See src/qcam/dng_writer.cpp, we record some colorspace-related information there. > >> Pixel X Dimension |1280 > >> Pixel Y Dimension |720 > >> > >> > >> > >> Sample output (2): exiv2 frame-stream0-000000.jpg > >> File name : frame-stream0-000000.jpg > >> File size : 38787 Bytes > >> MIME type : image/jpeg > >> Image size : 1280 x 720 > >> Camera make : Libcamera > >> Camera model : Camera2111 > >> Image timestamp : Sun Oct 13 07:03:20 734137 > >> > >> Image number : > > > > Hrm, I wonder if we should put the sequence number in here. > > > >> Exposure time : > >> Aperture : > >> Exposure bias : > >> Flash : > >> Flash bias : > >> Focal length : > >> Subject distance: > >> ISO speed : > >> Exposure mode : > >> Metering mode : > >> Macro mode : > >> Image quality : > > > > Plenty of parameters we'll want to get from the IPAs there, but that > > will be later. > > > >> Exif Resolution : 1280 x 720 > >> White balance : > >> Thumbnail : None > >> Copyright : > >> Exif comment : > >> > >> There are still a lot of tags that needs to be populated, > >> that can be built on top of this groundwork. > >> > >> [1]: https://github.com/kbingham/libcamera/commit/10ed8ec > >> > >> Kieran Bingham (1): > >> libcamera: android: Add EXIF infrastructure > >> > >> Umang Jain (2): > >> android: jpeg: Pass a Exif object while encoding > >> android: Support initial set of EXIF metadata tags > >> > >> src/android/camera_device.cpp | 12 +- > >> src/android/jpeg/encoder.h | 5 +- > >> src/android/jpeg/encoder_libjpeg.cpp | 15 +- > >> src/android/jpeg/encoder_libjpeg.h | 3 +- > >> src/android/jpeg/exif.cpp | 210 +++++++++++++++++++++++++++ > >> src/android/jpeg/exif.h | 53 +++++++ > >> src/android/meson.build | 2 + > >> 7 files changed, 296 insertions(+), 4 deletions(-) > >> create mode 100644 src/android/jpeg/exif.cpp > >> create mode 100644 src/android/jpeg/exif.h
Hi Laurent, On 8/27/20 11:14 PM, Laurent Pinchart wrote: > Hi Umang, > > On Thu, Aug 27, 2020 at 05:53:54AM +0000, Umang Jain wrote: >> On 8/25/20 2:58 AM, Kieran Bingham wrote: >>> On 24/08/2020 21:46, Umang Jain wrote: >>>> Hi all, >>>> >>>> This patch series targets adding initial EXIF metadata support >>>> for the frames captured in Android/Libcamera framework. >>>> >>>> It is initial in the sense, it adds the base infrastructure for >>>> Exif metadata support via 'libexif' and also encodes a initial >>>> set of metadata tags into the frames. >>>> >>>> Testing: >>>> The testing of this work was carried out via vivid pipeline-handler >>>> in conjunction with patch in `cam` using EncoderLibJpeg as the >>>> file sink [1]. That way we could see what EXIF metadata could be written >>>> and inspect that `exif` or `exiv2` commandline tools. >>>> >>>> ($) ./build/src/cam/cam -c vivid -C5 -F -s pixelformat=NV12 >>>> >>>> Sample output (1): exif frame-stream0-000000.jpg >>>> EXIF tags in 'frame-stream0-000000.jpg' ('Intel' byte order): >>>> --------------------+---------------------------------------------------------- >>>> Tag |Value >>>> --------------------+---------------------------------------------------------- >>>> Image Width |1280 >>>> Image Length |720 >>>> Manufacturer |Libcamera >>> Looking at my branch, it was of course me who put this in (incorrectly) >>> with a capital L, but please make it lowercase ;-) >>> >>>> Model |Camera2111 >>> I assume you set this Camera2111 string manually for testing ? >>> >>>> Orientation |Bottom-left >>>> X-Resolution |72 >>>> Y-Resolution |72 >>>> Resolution Unit |Inch >>>> Date and Time |Sun Oct 13 07:03:20 734137 >>>> >>>> YCbCr Positioning |Centered >>>> Exif Version |Exif Version 2.1 >>>> Date and Time (Origi|Sun Oct 13 07:03:20 734137 >>>> >>>> Date and Time (Digit|Sun Oct 13 07:03:20 734137 >>> Hrm ... we might have some work to do on converting the timestamp values >>> appropriately here. That doesn't look right at all ;-) >>> >>>> Components Configura|Y Cb Cr - >>>> FlashPixVersion |FlashPix Version 1.0 >>>> Color Space |Internal error (unknown value 65535) >>> This one needs to be checked if there is an error/incorrect parameter set. >> I found out that, this is not an error. The error messsage however, is >> misleading. >> https://github.com/Aries85/LightZone/issues/171#issuecomment-195730879 >> >> Color Space 65535 => Uncalibrated. >> We are running exif_data_fix(data_); in exif's constructor, so I think it's where >> this "Uncalibrated" value is getting set as the default value for color space. >> >> Can we query colorspace as of now? Then we can try setting it here as well. > See src/qcam/dng_writer.cpp, we record some colorspace-related > information there. Ah thanks for the pointer. However, it seems the values are somewhat implied and hardcoded for the TIFF_PHOTOMETRIC, which I suppose is the tag for conveying color space information. PHOTOMETRIC_RGB for the thumbnail and PHOTOMETRIC_CFA for the raw image. However, I just come across std::map<PixelFormat, JPEGPixelFormatInfo> which wraps colorspace information in src/android/jpeg/encoder_libjpeg.cpp Kieran, can we somehow use it in order to set the ColorSpace? The problem here is we are setting the tags one level up (in src/android/CameraDevice.cpp) so we might to relay the information at that point. What do you think? Also, I hope it can be follow up patch, not necessarily blocking this patchset :-) > >>>> Pixel X Dimension |1280 >>>> Pixel Y Dimension |720 >>>> >>>> >>>> >>>> Sample output (2): exiv2 frame-stream0-000000.jpg >>>> File name : frame-stream0-000000.jpg >>>> File size : 38787 Bytes >>>> MIME type : image/jpeg >>>> Image size : 1280 x 720 >>>> Camera make : Libcamera >>>> Camera model : Camera2111 >>>> Image timestamp : Sun Oct 13 07:03:20 734137 >>>> >>>> Image number : >>> Hrm, I wonder if we should put the sequence number in here. >>> >>>> Exposure time : >>>> Aperture : >>>> Exposure bias : >>>> Flash : >>>> Flash bias : >>>> Focal length : >>>> Subject distance: >>>> ISO speed : >>>> Exposure mode : >>>> Metering mode : >>>> Macro mode : >>>> Image quality : >>> Plenty of parameters we'll want to get from the IPAs there, but that >>> will be later. >>> >>>> Exif Resolution : 1280 x 720 >>>> White balance : >>>> Thumbnail : None >>>> Copyright : >>>> Exif comment : >>>> >>>> There are still a lot of tags that needs to be populated, >>>> that can be built on top of this groundwork. >>>> >>>> [1]: https://github.com/kbingham/libcamera/commit/10ed8ec >>>> >>>> Kieran Bingham (1): >>>> libcamera: android: Add EXIF infrastructure >>>> >>>> Umang Jain (2): >>>> android: jpeg: Pass a Exif object while encoding >>>> android: Support initial set of EXIF metadata tags >>>> >>>> src/android/camera_device.cpp | 12 +- >>>> src/android/jpeg/encoder.h | 5 +- >>>> src/android/jpeg/encoder_libjpeg.cpp | 15 +- >>>> src/android/jpeg/encoder_libjpeg.h | 3 +- >>>> src/android/jpeg/exif.cpp | 210 +++++++++++++++++++++++++++ >>>> src/android/jpeg/exif.h | 53 +++++++ >>>> src/android/meson.build | 2 + >>>> 7 files changed, 296 insertions(+), 4 deletions(-) >>>> create mode 100644 src/android/jpeg/exif.cpp >>>> create mode 100644 src/android/jpeg/exif.h
Hi Umang, On 28/08/2020 07:26, Umang Jain wrote: > Hi Laurent, > > On 8/27/20 11:14 PM, Laurent Pinchart wrote: >> Hi Umang, >> >> On Thu, Aug 27, 2020 at 05:53:54AM +0000, Umang Jain wrote: >>> On 8/25/20 2:58 AM, Kieran Bingham wrote: >>>> On 24/08/2020 21:46, Umang Jain wrote: >>>>> Hi all, >>>>> >>>>> This patch series targets adding initial EXIF metadata support >>>>> for the frames captured in Android/Libcamera framework. >>>>> >>>>> It is initial in the sense, it adds the base infrastructure for >>>>> Exif metadata support via 'libexif' and also encodes a initial >>>>> set of metadata tags into the frames. >>>>> >>>>> Testing: >>>>> The testing of this work was carried out via vivid pipeline-handler >>>>> in conjunction with patch in `cam` using EncoderLibJpeg as the >>>>> file sink [1]. That way we could see what EXIF metadata could be >>>>> written >>>>> and inspect that `exif` or `exiv2` commandline tools. >>>>> >>>>> ($) ./build/src/cam/cam -c vivid -C5 -F -s pixelformat=NV12 >>>>> >>>>> Sample output (1): exif frame-stream0-000000.jpg >>>>> EXIF tags in 'frame-stream0-000000.jpg' ('Intel' byte order): >>>>> --------------------+---------------------------------------------------------- >>>>> >>>>> Tag |Value >>>>> --------------------+---------------------------------------------------------- >>>>> >>>>> Image Width |1280 >>>>> Image Length |720 >>>>> Manufacturer |Libcamera >>>> Looking at my branch, it was of course me who put this in (incorrectly) >>>> with a capital L, but please make it lowercase ;-) >>>> >>>>> Model |Camera2111 >>>> I assume you set this Camera2111 string manually for testing ? >>>> >>>>> Orientation |Bottom-left >>>>> X-Resolution |72 >>>>> Y-Resolution |72 >>>>> Resolution Unit |Inch >>>>> Date and Time |Sun Oct 13 07:03:20 734137 >>>>> >>>>> YCbCr Positioning |Centered >>>>> Exif Version |Exif Version 2.1 >>>>> Date and Time (Origi|Sun Oct 13 07:03:20 734137 >>>>> >>>>> Date and Time (Digit|Sun Oct 13 07:03:20 734137 >>>> Hrm ... we might have some work to do on converting the timestamp >>>> values >>>> appropriately here. That doesn't look right at all ;-) >>>> >>>>> Components Configura|Y Cb Cr - >>>>> FlashPixVersion |FlashPix Version 1.0 >>>>> Color Space |Internal error (unknown value 65535) >>>> This one needs to be checked if there is an error/incorrect >>>> parameter set. >>> I found out that, this is not an error. The error messsage however, is >>> misleading. >>> https://github.com/Aries85/LightZone/issues/171#issuecomment-195730879 >>> >>> Color Space 65535 => Uncalibrated. >>> We are running exif_data_fix(data_); in exif's constructor, so I >>> think it's where >>> this "Uncalibrated" value is getting set as the default value for >>> color space. >>> >>> Can we query colorspace as of now? Then we can try setting it here as >>> well. >> See src/qcam/dng_writer.cpp, we record some colorspace-related >> information there. > Ah thanks for the pointer. > However, it seems the values are somewhat implied and hardcoded for > the TIFF_PHOTOMETRIC, which I suppose is the tag for conveying > color space information. PHOTOMETRIC_RGB for the thumbnail and > PHOTOMETRIC_CFA for the raw image. > > However, I just come across std::map<PixelFormat, JPEGPixelFormatInfo> > which wraps colorspace information in src/android/jpeg/encoder_libjpeg.cpp > Kieran, can we somehow use it in order to set the ColorSpace? The problem > here is we are setting the tags one level up (in > src/android/CameraDevice.cpp) > so we might to relay the information at that point. What do you think? > > Also, I hope it can be follow up patch, not necessarily blocking this > patchset > :-) Yes, this can be a follow on from my perspective. However it might not need any further action: Looking at: https://www.jeita.or.jp/japanese/standard/book/CP-3451E_E/#target/page_no=49 (Section 4.6.5-B) states the following: """ The color space information tag (ColorSpace) is always recorded as the color space specifier. Normally sRGB (=1) is used to define the color space based on the PC monitor conditions and environment. If a color space other than sRGB is used, Uncalibrated (=FFFF.H) is set. """ The question I don't yet know the answer to (color space is very complex, and possibly dependant upon the connected sensor attributes) is if we can assume we're in sRGB even though we are using YUV. Perhaps taking a look at the CrOS code to see what it does would be useful, but otherwise, I think we can leave this tag for now. >>>>> Pixel X Dimension |1280 >>>>> Pixel Y Dimension |720 >>>>> >>>>> >>>>> >>>>> Sample output (2): exiv2 frame-stream0-000000.jpg >>>>> File name : frame-stream0-000000.jpg >>>>> File size : 38787 Bytes >>>>> MIME type : image/jpeg >>>>> Image size : 1280 x 720 >>>>> Camera make : Libcamera >>>>> Camera model : Camera2111 >>>>> Image timestamp : Sun Oct 13 07:03:20 734137 >>>>> >>>>> Image number : >>>> Hrm, I wonder if we should put the sequence number in here. >>>> >>>>> Exposure time : >>>>> Aperture : >>>>> Exposure bias : >>>>> Flash : >>>>> Flash bias : >>>>> Focal length : >>>>> Subject distance: >>>>> ISO speed : >>>>> Exposure mode : >>>>> Metering mode : >>>>> Macro mode : >>>>> Image quality : >>>> Plenty of parameters we'll want to get from the IPAs there, but that >>>> will be later. >>>> >>>>> Exif Resolution : 1280 x 720 >>>>> White balance : >>>>> Thumbnail : None >>>>> Copyright : >>>>> Exif comment : >>>>> >>>>> There are still a lot of tags that needs to be populated, >>>>> that can be built on top of this groundwork. >>>>> >>>>> [1]: https://github.com/kbingham/libcamera/commit/10ed8ec >>>>> >>>>> Kieran Bingham (1): >>>>> libcamera: android: Add EXIF infrastructure >>>>> >>>>> Umang Jain (2): >>>>> android: jpeg: Pass a Exif object while encoding >>>>> android: Support initial set of EXIF metadata tags >>>>> >>>>> src/android/camera_device.cpp | 12 +- >>>>> src/android/jpeg/encoder.h | 5 +- >>>>> src/android/jpeg/encoder_libjpeg.cpp | 15 +- >>>>> src/android/jpeg/encoder_libjpeg.h | 3 +- >>>>> src/android/jpeg/exif.cpp | 210 >>>>> +++++++++++++++++++++++++++ >>>>> src/android/jpeg/exif.h | 53 +++++++ >>>>> src/android/meson.build | 2 + >>>>> 7 files changed, 296 insertions(+), 4 deletions(-) >>>>> create mode 100644 src/android/jpeg/exif.cpp >>>>> create mode 100644 src/android/jpeg/exif.h
Hi all, This patch series targets adding initial EXIF metadata support for the frames captured in Android/Libcamera framework. It is initial in the sense, it adds the base infrastructure for Exif metadata support via 'libexif' and also encodes a initial set of metadata tags into the frames. Testing: The testing of this work was carried out via vivid pipeline-handler in conjunction with patch in `cam` using EncoderLibJpeg as the file sink [1]. That way we could see what EXIF metadata could be written and inspect that `exif` or `exiv2` commandline tools. ($) ./build/src/cam/cam -c vivid -C5 -F -s pixelformat=NV12 Sample output (1): exif frame-stream0-000000.jpg EXIF tags in 'frame-stream0-000000.jpg' ('Intel' byte order): --------------------+---------------------------------------------------------- Tag |Value --------------------+---------------------------------------------------------- Image Width |1280 Image Length |720 Manufacturer |Libcamera Model |Camera2111 Orientation |Bottom-left X-Resolution |72 Y-Resolution |72 Resolution Unit |Inch Date and Time |Sun Oct 13 07:03:20 734137 YCbCr Positioning |Centered Exif Version |Exif Version 2.1 Date and Time (Origi|Sun Oct 13 07:03:20 734137 Date and Time (Digit|Sun Oct 13 07:03:20 734137 Components Configura|Y Cb Cr - FlashPixVersion |FlashPix Version 1.0 Color Space |Internal error (unknown value 65535) Pixel X Dimension |1280 Pixel Y Dimension |720 Sample output (2): exiv2 frame-stream0-000000.jpg File name : frame-stream0-000000.jpg File size : 38787 Bytes MIME type : image/jpeg Image size : 1280 x 720 Camera make : Libcamera Camera model : Camera2111 Image timestamp : Sun Oct 13 07:03:20 734137 Image number : Exposure time : Aperture : Exposure bias : Flash : Flash bias : Focal length : Subject distance: ISO speed : Exposure mode : Metering mode : Macro mode : Image quality : Exif Resolution : 1280 x 720 White balance : Thumbnail : None Copyright : Exif comment : There are still a lot of tags that needs to be populated, that can be built on top of this groundwork. [1]: https://github.com/kbingham/libcamera/commit/10ed8ec Kieran Bingham (1): libcamera: android: Add EXIF infrastructure Umang Jain (2): android: jpeg: Pass a Exif object while encoding android: Support initial set of EXIF metadata tags src/android/camera_device.cpp | 12 +- src/android/jpeg/encoder.h | 5 +- src/android/jpeg/encoder_libjpeg.cpp | 15 +- src/android/jpeg/encoder_libjpeg.h | 3 +- src/android/jpeg/exif.cpp | 210 +++++++++++++++++++++++++++ src/android/jpeg/exif.h | 53 +++++++ src/android/meson.build | 2 + 7 files changed, 296 insertions(+), 4 deletions(-) create mode 100644 src/android/jpeg/exif.cpp create mode 100644 src/android/jpeg/exif.h