On Mon, May 11, 2020 at 9:37 PM Neil Birkbeck <neil.birkb...@gmail.com>
wrote:
>
>
> On Wed, May 6, 2020 at 8:45 AM James Almer <jamr...@gmail.com> wrote:
>
>> On 5/6/2020 12:22 PM, Neil Birkbeck wrote:
>> > On Tue, May 5, 2020 at 5:11 AM Kieran O Leary <kieran.o.le...@gmail.com
>> >
>> > wrote:
>> >
>> >> Hi,
>> >>
>> >> I broke the threading with my last reply, i apologise. Here goes
>> another
>> >> attempt:
>> >>
>> >> On Tue, Apr 28, 2020 at 6:23 PM Neil Birkbeck <neil.birkb...@gmail.com
>> >
>> >> wrote:
>> >>
>> >>> On Tue, Apr 28, 2020 at 3:18 AM Nicolas George <geo...@nsup.org>
>> wrote:
>> >>>
>> >>>> Andreas Rheinhardt (12020-04-28):
>> >>>>> That's expected. The patch provided only provides the structure in
>> >>> which
>> >>>>> the values are intended to be exported; it does not add any demuxing
>> >> or
>> >>>>> muxing capabilities for mov or mkv (as you can see from the fact
>> that
>> >>>>> none of these (de)muxers have been changed in the patch).
>> >>>>
>> >>>> Which is something I intended to comment on: adding code without
>> users
>> >>>> is rarely a good idea. I suggest we do not commit until at least one
>> >>>> demuxer use it, preferably at least two. Otherwise, we may realize
>> that
>> >>>> “oh crap, it doesn't work” because of a tiny unforeseen detail.
>> >>>
>> >>>
>> >>> Thanks for the feedback. I also have patches for the demux (MOV/MKV)
>> and
>> >>> mux (MOV/MKV).
>> >>>
>> >>> As there is still the alternative of using the fields in the
>> >>> AVCodecParameters/AVCodecContext, my intention was to keep the first
>> >> patch
>> >>> small to resolve discussion on that point.
>> >>>
>> >>> I've included the patches, if you'd like to try test it, Kieren. I
>> see on
>> >>> your test file that there may be some slight rounding error making
>> output
>> >>> crop 704 not 703 (MKV file ends up with pixel_crop_{left,right} = 8).
>> >>>
>> >>> /ffprobe ../testdata/clap.mov 2>&1 | grep -A1 "Side"
>> >>> Side data:
>> >>> Clean aperture:[width 41472/59 height:576/1 h_offset:0/1
>> >>> v_offset:0/1]
>> >>> ./ffmpeg -i ../testdata/clap.mov -vcodec copy -acodec copy
>> /tmp/clap.mkv
>> >>> ./ffprobe /tmp/clap.mkv 2>&1 | grep -A1 "Side"
>> >>> Side data:
>> >>> Clean aperture:[width 704/1 height:576/1 h_offset:0/1
>> v_offset:0/1]
>> >>>
>> >>
>> >> I have to look deeper into the MKV side of things and most likely
>> raise it
>> >> with the cellar mailing list so that better minds than mine can weigh
>> in. I
>> >> do see that the rounding up to 704 could be an issue alright.
>> >> As for MOV, your patch appears to generate the same output clap values
>> as
>> >> the input, so that's really great! command line and mediainfo trace
>> below:
>> >>
>> >
>> > Thanks for testing, Kieran and for linking the discussion on the cellar
>> > list.
>> >
>> > Any additional thoughts from ffmpeg devs on container-level SideData vs
>> > adding the extra fields into AVCodecParameters/AVCodecContext and
>> plumbing
>> > into the frame instead? I anticipate some situations where there can be
>> > interaction between cropping in bitstream and container-level cropping.
>> > Maybe the best way forward is for me to share some sample patches for
>> the
>> > alternative to validate whether it supports the various use cases
>> > (transmux, decode+crop, any other application level handling), and to
>> > confirm the interface changes for the structs.
>>
>> One option could be to also introduce a frame side data for this new
>> struct and have it replace the AVFrame fields, which would then be
>> deprecated (But of course keep working until removed).
>> This would allow to either inject stream side data from clap atoms and
>> Matroska crop fields into packets (Which would then be propagated to
>> frames to be applied during decoding), or use and export the bitstream
>> cropping information as it's the case right now, all while preventing
>> the addition of new fields to AVCodecParameters.
>>
>>
> I agree that sharing the SideData with the frame could be nice for the
> above reasons. I guess any interaction or conflict between bitstream &
> container cropping could then get resolved at the decoder (e.g., assuming
> that say SPS from h264 and container-level cropping should be composed).
>
>
>> I would like a developer that makes use of this feature to also comment,
>> especially seeing how the AVFrame fields and this clap side data are
>> pretty different.
>>
>
> The current CleanAperture representation was in part motivated to 1) keep
> the representation capable of representing the CLAP atom (same
> representation and rationals), and 2) to make it unambiguous that this was
> container-level stream metadata. The representation is a tiny bit more
> tedious when trying to actually perform a crop (e.g., to extract the
> top-left corner offset). If sharing as AVFrame side data, a representation
> closer to AVFrame's crop_{top,bottom,left,right} may be more natural.
>
> Within ffmpeg, I see that the existing codecs using AVFrame cropping
> include:
> -libavcodec/h264_slice.c: propagating the crop_fields from the SPS in h264
> in
> -libavcodec/hevc_refs.c: (sly to h264)
> -libavcodec/agm.c: crop_{left, top} inferred from
> avctx->coded_{width,height} - avctx->{width,height}
> -libavcodec/videotoolbox.c: crop fields explicitly set to zero.
> -libvacodec/vp3.c: crop_{left,top} set from coded bitstream’s offset_{x,y}
> (right,bottom from coded_{width,height})
>
> Upon review, I see there is one other format that appears to export
> similar cropping information into key/value pair metadata that could
> potentially also use the stream-level side data: (libavformat/cinedec.c:
> set_metadata_int(&st->metadata, "crop_left", avio_rl32(pb), 1))
>
I've changed the side data to be PixelCrop (instead of CleanAperture) given
the intent to reuse as cropping elsewhere.
-For now, I've kept the rational representation--although CLAP seems to be
the only required case of it. Maybe Kieran could comment on the requirement
of having maintaining the rationals for CLAP (only works on mov to mov
transmuxing).
Any other feedback on the representation?
To test crop on decode behavior, I've passed the metadata through to frame
data. Two minor observations:
1) With the current implementation of inject_global_side_data, the metadata
only makes it onto the first frame. Will need a special case to make sure
the metadata makes it onto every frame in libavformat/utils.c.
2) Cropping in libavutil/frame.c is adjusting frame pointers and by default
needs to respect alignment (unlike say the autorotate approach of adding a
vf_crop filter in ./fftools/ffmpeg_filter.c)
I also have updates to the demuxer/muxer patches, including incorporating
Andreas' MKV feedback, but will wait sending patches until we've resolved
the representation.
From f1c94678c2e8cfec074c7340c4ae5ad976a88504 Mon Sep 17 00:00:00 2001
From: Neil Birkbeck <neil.birkb...@gmail.com>
Date: Sun, 26 Apr 2020 19:47:57 -0700
Subject: [PATCH 1/6] libavutil: add pixel crop / clean aperture (CLAP) side
data.
First step towards demuxing CLAP atom metadata, helping to resolve:
https://trac.ffmpeg.org/ticket/7437
This PixelCrop representation can losslessly carry CLAP metadata.
And it can also carry PixelCrop fields from MKV:
http://ffmpeg.org/pipermail/ffmpeg-devel/2020-March/259333.html
Signed-off-by: Neil Birkbeck <neil.birkb...@gmail.com>
---
libavcodec/avpacket.c | 1 +
libavcodec/packet.h | 12 ++++++++
libavformat/dump.c | 17 ++++++++++++
libavutil/Makefile | 2 ++
libavutil/pixel_crop.c | 30 ++++++++++++++++++++
libavutil/pixel_crop.h | 62 ++++++++++++++++++++++++++++++++++++++++++
6 files changed, 124 insertions(+)
create mode 100644 libavutil/pixel_crop.c
create mode 100644 libavutil/pixel_crop.h
diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c
index 033f2d8f26..1bb15d6ca1 100644
--- a/libavcodec/avpacket.c
+++ b/libavcodec/avpacket.c
@@ -399,6 +399,7 @@ const char *av_packet_side_data_name(enum AVPacketSideDataType type)
case AV_PKT_DATA_PRFT: return "Producer Reference Time";
case AV_PKT_DATA_ICC_PROFILE: return "ICC Profile";
case AV_PKT_DATA_DOVI_CONF: return "DOVI configuration record";
+ case AV_PKT_DATA_PIXEL_CROP: return "Pixel Crop (CLAP) metadata";
}
return NULL;
}
diff --git a/libavcodec/packet.h b/libavcodec/packet.h
index 41485f4527..02732e877b 100644
--- a/libavcodec/packet.h
+++ b/libavcodec/packet.h
@@ -282,6 +282,18 @@ enum AVPacketSideDataType {
*/
AV_PKT_DATA_DOVI_CONF,
+ /**
+ * This side data contains a crop region (aka clean aperture) that defines
+ * the region of the stored image that should be cropped.
+ *
+ * It is intended to be used to store crop-related metadata from the
+ * container. For example, the CLAP atom in MOV files, and PixelCrop fields
+ * in MKV.
+ *
+ * The data is of type AVPixelCrop (see libavutil/pixel_crop.h)
+ */
+ AV_PKT_DATA_PIXEL_CROP,
+
/**
* The number of side data types.
* This is not part of the public API/ABI in the sense that it may
diff --git a/libavformat/dump.c b/libavformat/dump.c
index 06bafc272d..26d8df259f 100644
--- a/libavformat/dump.c
+++ b/libavformat/dump.c
@@ -31,6 +31,7 @@
#include "libavutil/mathematics.h"
#include "libavutil/opt.h"
#include "libavutil/avstring.h"
+#include "libavutil/pixel_crop.h"
#include "libavutil/replaygain.h"
#include "libavutil/spherical.h"
#include "libavutil/stereo3d.h"
@@ -402,6 +403,18 @@ static void dump_dovi_conf(void *ctx, AVPacketSideData* sd)
dovi->dv_bl_signal_compatibility_id);
}
+static void dump_pixel_crop(void *ctx, AVPacketSideData *sd)
+{
+ AVPixelCrop *crop = (AVPixelCrop *)sd->data;
+ av_log(ctx, AV_LOG_INFO,
+ "[crop_left=%d/%d crop_top=%d/%d "
+ "crop_right=%d/%d crop_bottom=%d/%d]",
+ crop->crop_left.num, crop->crop_left.den,
+ crop->crop_top.num, crop->crop_top.den,
+ crop->crop_right.num, crop->crop_right.den,
+ crop->crop_bottom.num, crop->crop_bottom.den);
+}
+
static void dump_sidedata(void *ctx, AVStream *st, const char *indent)
{
int i;
@@ -468,6 +481,10 @@ static void dump_sidedata(void *ctx, AVStream *st, const char *indent)
av_log(ctx, AV_LOG_INFO, "DOVI configuration record: ");
dump_dovi_conf(ctx, &sd);
break;
+ case AV_PKT_DATA_PIXEL_CROP:
+ av_log(ctx, AV_LOG_INFO, "Pixel crop:");
+ dump_pixel_crop(ctx, &sd);
+ break;
default:
av_log(ctx, AV_LOG_INFO,
"unknown side data type %d (%d bytes)", sd.type, sd.size);
diff --git a/libavutil/Makefile b/libavutil/Makefile
index 9b08372eb2..760df8edea 100644
--- a/libavutil/Makefile
+++ b/libavutil/Makefile
@@ -60,6 +60,7 @@ HEADERS = adler32.h \
murmur3.h \
opt.h \
parseutils.h \
+ pixel_crop.h \
pixdesc.h \
pixelutils.h \
pixfmt.h \
@@ -145,6 +146,7 @@ OBJS = adler32.o \
parseutils.o \
pixdesc.o \
pixelutils.o \
+ pixel_crop.o \
random_seed.o \
rational.o \
reverse.o \
diff --git a/libavutil/pixel_crop.c b/libavutil/pixel_crop.c
new file mode 100644
index 0000000000..3097add86f
--- /dev/null
+++ b/libavutil/pixel_crop.c
@@ -0,0 +1,30 @@
+/**
+ * Copyright (c) 2020 Neil Birkbeck <neil.birkb...@gmail.com>
+ *
+ * This file is part of FFmpeg.
+ *
+ * FFmpeg is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * FFmpeg is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with FFmpeg; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
+ */
+
+#include <stdint.h>
+#include <string.h>
+
+#include "pixel_crop.h"
+#include "mem.h"
+
+AVPixelCrop *av_pixel_crop_alloc(void)
+{
+ return av_mallocz(sizeof(AVPixelCrop));
+}
diff --git a/libavutil/pixel_crop.h b/libavutil/pixel_crop.h
new file mode 100644
index 0000000000..e1e5df6527
--- /dev/null
+++ b/libavutil/pixel_crop.h
@@ -0,0 +1,62 @@
+/*
+ * Copyright (c) 2020 Neil Birkbeck <neil.birkb...@gmail.com>
+ *
+ * This file is part of FFmpeg.
+ *
+ * FFmpeg is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * FFmpeg is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with FFmpeg; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
+ */
+
+#ifndef AVUTIL_PIXEL_CROP_H
+#define AVUTIL_PIXEL_CROP_H
+
+#include "frame.h"
+#include "rational.h"
+
+
+/**
+ * Pixel crop side data represents the subset of stored pixels that should
+ * be cropped off the stored image.
+ *
+ * The structure is meant to be meant to be allocated as stream side data
+ * and is capable of losslessly representing the Clean Aperture Atom (CLAP)
+ * from MOV files (which stores entries as rational) and container-level
+ * cropping information (e.g., PixelCrop fields in MKV).
+ *
+ * @note The struct should be allocated with av_pixel_crop_alloc()
+ * and its size is not a part of the public ABI.
+ */
+typedef struct AVPixelCrop {
+ // The number of pixels to crop off the left of the image.
+ AVRational crop_left;
+
+ // The number of pixels to crop off the top of the image.
+ AVRational crop_top;
+
+ // The number of pixels to crop off the right of the image.
+ AVRational crop_right;
+
+ // The number of pixels to crop off the bottom of the image.
+ AVRational crop_bottom;
+} AVPixelCrop;
+
+/**
+ * Allocate an AVPixelCrop structure and set its fields to default values.
+ * The resulting struct can be freed using av_freep().
+ *
+ * @return An AVPixelCrop filled with default values or NULL on failure.
+ */
+AVPixelCrop *av_pixel_crop_alloc(void);
+
+#endif /* AVUTIL_PIXEL_CROP_H */
--
2.27.0.rc0.183.gde8f92d652-goog
_______________________________________________
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".
