Quoting Leo Izen (2024-09-16 16:43:42)
> This patch centralizes much of the EXIF parsing and handling code for
> libavcodec, and delegates its own AVFrameSideData type to containing
> the buffer that holds EXIF metadata. This patch also adds exposes the
> exif parsing routing so it can be called by ffprobe, and updates the
> corresponding FATE tests to read the keys from the side data instead of
> from the main frame metadata.
Does that mean you're no longer exporting frame metadata? That would
break existing callers that are reading it.
> This commit also deprecates an avpriv_ function in exif.h, exposing the
> parsing functionality as a public API in the exported libavcodec/exif.h
> header file.
>
> Signed-off-by: Leo Izen <leo.i...@gmail.com>
> ---
> fftools/ffprobe.c | 27 +-
> libavcodec/Makefile | 1 +
> libavcodec/exif.c | 394 +++++++++++++++++++++++++++--
> libavcodec/exif.h | 32 ++-
> libavcodec/exif_internal.h | 55 ++++
> libavcodec/mjpegdec.c | 91 +------
> libavcodec/mjpegdec.h | 2 +-
> libavcodec/tiff.c | 19 +-
> libavcodec/tiff.h | 1 +
> libavcodec/version.h | 2 +-
> libavcodec/webp.c | 38 +--
> libavformat/avidec.c | 4 +-
> libavutil/frame.c | 2 +
> libavutil/frame.h | 6 +
> tests/ref/fate/exif-image-embedded | 5 +-
> tests/ref/fate/exif-image-jpg | 91 +++----
> tests/ref/fate/exif-image-webp | 91 +++----
> 17 files changed, 631 insertions(+), 230 deletions(-)
> create mode 100644 libavcodec/exif_internal.h
As James said, it's a bit too many changes at once. I'd split it into:
* add new side data type
* add lavc public API
* codec changes
* lavf changes
* ffprobe changes
> diff --git a/libavcodec/exif_internal.h b/libavcodec/exif_internal.h
> new file mode 100644
> index 0000000000..2c788856d6
> --- /dev/null
> +++ b/libavcodec/exif_internal.h
> +/**
> + * Attach an AVBufferRef containing EXIF metadata to a frame.
> + * This function allocates the necessary AVFrameSideData and attaches it,
> + * and also attaches any necessary other sidedata that can be read from the,
> + * EXIF metadata, such as a display matrix.
> + */
> +int ff_exif_attach(void *logctx, AVFrame *frame, AVBufferRef **data);
Tiff and webp can leak data if this function fails. You should probably
declare that it always takes ownership of data and unref it on failure.
> +
> +/**
> + * Collects an IFD into a buffer. **buffer points to an AVBufferRef *, which
> will
> + * be allocated by this function. The caller needs to unref the buffer when
> it is
> + * done with it. This function also writes the EXIF/TIFF header into the
> buffer
> + * based on the endianness, so encoders can pass the buffer as-is. This
> function can be
> + * used by TIFF or other codecs that have non-zero IFD offsets on their EXIF
> metadata.
> + */
> +int ff_exif_collect_ifd(void *logctx, GetByteContext *gb, int le,
> AVBufferRef **buffer);
> +
> +#endif /* AVCODEC_EXIF_INTERNAL_H */
> diff --git a/libavcodec/mjpegdec.c b/libavcodec/mjpegdec.c
> index 86ec58713c..9ed71976df 100644
> --- a/libavcodec/mjpegdec.c
> +++ b/libavcodec/mjpegdec.c
> @@ -43,6 +43,7 @@
> #include "codec_internal.h"
> #include "copy_block.h"
> #include "decode.h"
> +#include "exif_internal.h"
> #include "hwaccel_internal.h"
> #include "hwconfig.h"
> #include "idctdsp.h"
> @@ -2039,8 +2040,6 @@ static int mjpeg_decode_app(MJpegDecodeContext *s)
>
> /* EXIF metadata */
> if (s->start_code == APP1 && id == AV_RB32("Exif") && len >= 2) {
> - GetByteContext gbytes;
> - int ret, le, ifd_offset, bytes_read;
> const uint8_t *aligned;
>
> skip_bits(&s->gb, 16); // skip padding
> @@ -2048,27 +2047,12 @@ static int mjpeg_decode_app(MJpegDecodeContext *s)
>
> // init byte wise reading
> aligned = align_get_bits(&s->gb);
> - bytestream2_init(&gbytes, aligned, len);
> -
> - // read TIFF header
> - ret = ff_tdecode_header(&gbytes, &le, &ifd_offset);
> - if (ret) {
> - av_log(s->avctx, AV_LOG_ERROR, "mjpeg: invalid TIFF header in
> EXIF data\n");
> - } else {
> - bytestream2_seek(&gbytes, ifd_offset, SEEK_SET);
> -
> - // read 0th IFD and store the metadata
> - // (return values > 0 indicate the presence of subimage metadata)
> - ret = ff_exif_decode_ifd(s->avctx, &gbytes, le, 0,
> &s->exif_metadata);
> - if (ret < 0) {
> - av_log(s->avctx, AV_LOG_ERROR, "mjpeg: error decoding EXIF
> data\n");
> - }
> - }
> -
> - bytes_read = bytestream2_tell(&gbytes);
> - skip_bits(&s->gb, bytes_read << 3);
> - len -= bytes_read;
> -
> + s->exif_buffer = av_buffer_alloc(len);
Is it guaranteed s->exif_buffer is NULL at this point, so you're not
leaking anything?
> diff --git a/libavcodec/tiff.c b/libavcodec/tiff.c
> index 37b56e9757..45c3ae8c97 100644
> --- a/libavcodec/tiff.c
> +++ b/libavcodec/tiff.c
> @@ -46,6 +46,7 @@
> #include "bytestream.h"
> #include "codec_internal.h"
> #include "decode.h"
> +#include "exif_internal.h"
> #include "faxcompr.h"
> #include "lzw.h"
> #include "tiff.h"
> @@ -1268,7 +1269,7 @@ static int tiff_decode_tag(TiffContext *s, AVFrame
> *frame)
> s->last_tag = tag;
>
> off = bytestream2_tell(&s->gb);
> - if (count == 1) {
> + if (count == 1 && tag != TIFF_EXIFTAG) {
> switch (type) {
> case TIFF_BYTE:
> case TIFF_SHORT:
> @@ -1770,6 +1771,22 @@ static int tiff_decode_tag(TiffContext *s, AVFrame
> *frame)
> case TIFF_SOFTWARE_NAME:
> ADD_METADATA(count, "software", NULL);
> break;
> + case TIFF_EXIFTAG: {
> + AVBufferRef *exif = NULL;
> + int next;
> + gb_temp = s->gb;
> + next = ff_exif_collect_ifd(s->avctx, &gb_temp, s->le, &exif);
> + if (next < 0)
> + av_log(s->avctx, AV_LOG_ERROR, "Error parsing TIFF exif tags:
> %d\n", next);
> + else if (next)
> + bytestream2_seek(&s->gb, next, SEEK_SET);
> + if (exif) {
> + ret = ff_exif_attach(s->avctx, frame, &exif);
> + if (ret < 0)
> + return ret;
> + }
> + break;
> + }
Is this adding support for something that was not parsed before? Should
probably also be a separate commit.
--
Anton Khirnov
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
