Re: [FFmpeg-devel] [PATCH] libavutil: add clean aperture (CLAP) side data.
On Mon, Jun 8, 2020 at 7:19 AM Tobias Rapp wrote:
> On 30.05.2020 12:41, Kieran O Leary wrote:
> > Hi,
> >
> > On Fri 29 May 2020, 22:47 Neil Birkbeck,
> wrote:
> >
> >> [...]
> >> 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).
> >>
> > I'm no expert, but I think a lot of this just comes from video standards
> > that stipulate those rational numbers? I've cc'd tobias Rapp and
> Christoph
> > Gerstbauer of NOA just to bring this to their attention, as I'm pretty
> sure
> > that they use cropping values in AVI, so perhaps all of this could be
> > useful to them in some way.
> >
>
> Hi Kieran,
>
> when digitizing SD video carriers we indeed store some offset
> information in case VBI is preverved (i.e. PAL 720x608). But these
> offset values are currently not stored in the AVI container itself. The
> OpenDML "vprp" chunk defines some offset values but for the purpose of
> compressed image data where the codec implies some multiple-of-N
> height/width dimension on the data. So it did not seem to match our
> use-case.
>
> Besides AVI and the mentioned MKV and MOV formats I remember some
> display offset/cropping information being stored in MXF with the Display
> X/Y-Offset values.
>
> Regardless whether the frame crop/offset values are stored as frame
> fields or side data: how would this information be affected by filters
> like "crop" or "scale"?
>
Thanks for the additional info, Kieran and Tobias.
AVFrame already has cropping fields (AVFrame::crop_{top, bottom, left,
right}), and James Almer's suggestion was that this representation could
replace those. At the moment, when AVCodecContext::apply_cropping is true,
these offsets get applied (and zerod) in libavcodec/decode.c. Currently,
vf_crop adjusts these offsets for HWACCEL pix fmts, and vf_scale_vulkan
also respects (vf_scale doesn't).
Any additional reviewer thoughts on the SideData representation (e.g., crop
fields, currently rational to fully support the mov mux case)?
___
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".
Re: [FFmpeg-devel] [PATCH] libavutil: add clean aperture (CLAP) side data.
On 30.05.2020 12:41, Kieran O Leary wrote: Hi, On Fri 29 May 2020, 22:47 Neil Birkbeck, wrote: [...] 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). I'm no expert, but I think a lot of this just comes from video standards that stipulate those rational numbers? I've cc'd tobias Rapp and Christoph Gerstbauer of NOA just to bring this to their attention, as I'm pretty sure that they use cropping values in AVI, so perhaps all of this could be useful to them in some way. Hi Kieran, when digitizing SD video carriers we indeed store some offset information in case VBI is preverved (i.e. PAL 720x608). But these offset values are currently not stored in the AVI container itself. The OpenDML "vprp" chunk defines some offset values but for the purpose of compressed image data where the codec implies some multiple-of-N height/width dimension on the data. So it did not seem to match our use-case. Besides AVI and the mentioned MKV and MOV formats I remember some display offset/cropping information being stored in MXF with the Display X/Y-Offset values. Regardless whether the frame crop/offset values are stored as frame fields or side data: how would this information be affected by filters like "crop" or "scale"? Regards, Tobias ___ 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".
Re: [FFmpeg-devel] [PATCH] libavutil: add clean aperture (CLAP) side data.
Hi,
On Fri 29 May 2020, 22:47 Neil Birkbeck, wrote:
> On Mon, May 11, 2020 at 9:37 PM Neil Birkbeck
> wrote:
>
> >
> >
> > On Wed, May 6, 2020 at 8:45 AM James Almer 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
> >> 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
Re: [FFmpeg-devel] [PATCH] libavutil: add clean aperture (CLAP) side data.
On Mon, May 11, 2020 at 9:37 PM Neil Birkbeck
wrote:
>
>
> On Wed, May 6, 2020 at 8:45 AM James Almer wrote:
>
>> On 5/6/2020 12:22 PM, Neil Birkbeck wrote:
>> > On Tue, May 5, 2020 at 5:11 AM Kieran O Leary > >
>> > 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 > >
>> >> wrote:
>> >>
>> >>> On Tue, Apr 28, 2020 at 3:18 AM Nicolas George
>> 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
Re: [FFmpeg-devel] [PATCH] libavutil: add clean aperture (CLAP) side data.
On Wed, May 6, 2020 at 8:45 AM James Almer wrote:
> On 5/6/2020 12:22 PM, Neil Birkbeck wrote:
> > On Tue, May 5, 2020 at 5:11 AM Kieran O Leary
> > 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
> >> wrote:
> >>
> >>> On Tue, Apr 28, 2020 at 3:18 AM Nicolas George
> 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
Re: [FFmpeg-devel] [PATCH] libavutil: add clean aperture (CLAP) side data.
On Thu, May 7, 2020 at 10:21 AM Andreas Rheinhardt <
andreas.rheinha...@gmail.com> wrote:
> Neil Birkbeck:
> > On Tue, Apr 28, 2020 at 3:18 AM Nicolas George 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]
> >
> >
>
> >
> > Write out stream-level AVCleanAperture side data in the muxer.
>
> You should separate the mov and Matroska patches.
I will do this as I incorporate the rest of your suggestions.
>
> > Signed-off-by: Neil Birkbeck
> > ---
> > libavformat/matroskaenc.c | 37 +
> > libavformat/movenc.c | 28
> > 2 files changed, 57 insertions(+), 8 deletions(-)
> >
> > diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c
> > index 784973a951..19ff29853e 100644
> > --- a/libavformat/matroskaenc.c
> > +++ b/libavformat/matroskaenc.c
> > @@ -901,6 +901,38 @@ static int mkv_write_video_color(AVIOContext *pb,
> const AVCodecParameters *par,
> > return 0;
> > }
> >
> > +static int mkv_write_video_crop(AVIOContext *pb, AVCodecParameters
> *par, AVStream *st)
> > +{
> > +int cropb = 0, cropt = 0, cropl = 0, cropr = 0;
> > +double width, height, h_offset, v_offset;
> > +int side_data_size = 0;
> > +const AVCleanAperture *clap =
> > +(const AVCleanAperture *)av_stream_get_side_data(
> > +st, AV_PKT_DATA_CLEAN_APERTURE, _data_size);
> > +if (!clap)
> > +return 0;
> > +
> > +width = av_q2d(clap->width);
> > +height = av_q2d(clap->height);
> > +h_offset = av_q2d(clap->horizontal_offset);
> > +v_offset = av_q2d(clap->vertical_offset);
> > +cropb = (int)(par->height - (par->height / 2. + v_offset + height /
> 2));
> > +cropt = (int)(par->height / 2. + v_offset - height / 2);
> > +cropr = (int)(par->width - (par->width / 2. + h_offset + width /
> 2));
> > +cropl = (int)(par->width / 2. + h_offset - width / 2);
> > +cropb = FFMAX(cropb, 0);
> > +cropt = FFMAX(cropt, 0);
> > +cropr = FFMAX(cropr, 0);
> > +cropl = FFMAX(cropl, 0);
> > +if (!cropr && !cropl && !cropt && !cropb)
> > +return 0;
> > +put_ebml_uint(pb, MATROSKA_ID_VIDEOPIXELCROPL, cropl);
> > +put_ebml_uint(pb, MATROSKA_ID_VIDEOPIXELCROPR, cropr);
> > +put_ebml_uint(pb, MATROSKA_ID_VIDEOPIXELCROPB, cropb);
> > +put_ebml_uint(pb, MATROSKA_ID_VIDEOPIXELCROPT, cropt);
>
> There is no reason to write all four elements when only some of them are
> different from zero.
>
>
Good point. I will avoid doing this in the revised set of patches.
> > +return 0;
> > +}
> > +
> > static int mkv_write_video_projection(AVFormatContext *s, AVIOContext
> *pb,
> >const AVStream *st)
> > {
> > @@ -1287,6 +1319,11 @@ static int mkv_write_track(AVFormatContext *s,
> MatroskaMuxContext *mkv,
> > } else if (mkv->mode != MODE_WEBM)
> > put_ebml_uint(pb, MATROSKA_ID_VIDEODISPLAYUNIT,
> MATROSKA_VIDEO_DISPLAYUNIT_UNKNOWN);
> >
> > +// Write out pixel crop
> > +ret = mkv_write_video_crop(pb, par, st);
> > +if (ret < 0)
> > +return ret;
>
> This is the wrong place for this: It needs to be performed before we
> write the display dimensions and you need to return the cropped width
> and height via pointer arguments, so that the code doing display
> dimensions (which needs to be updated) knows about the cropping. This is
> necessary, because cropping is supposed to happen
Re: [FFmpeg-devel] [PATCH] libavutil: add clean aperture (CLAP) side data.
Neil Birkbeck:
> On Tue, Apr 28, 2020 at 3:18 AM Nicolas George 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]
>
>
>
> Write out stream-level AVCleanAperture side data in the muxer.
You should separate the mov and Matroska patches.
>
> Signed-off-by: Neil Birkbeck
> ---
> libavformat/matroskaenc.c | 37 +
> libavformat/movenc.c | 28
> 2 files changed, 57 insertions(+), 8 deletions(-)
>
> diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c
> index 784973a951..19ff29853e 100644
> --- a/libavformat/matroskaenc.c
> +++ b/libavformat/matroskaenc.c
> @@ -901,6 +901,38 @@ static int mkv_write_video_color(AVIOContext *pb, const
> AVCodecParameters *par,
> return 0;
> }
>
> +static int mkv_write_video_crop(AVIOContext *pb, AVCodecParameters *par,
> AVStream *st)
> +{
> +int cropb = 0, cropt = 0, cropl = 0, cropr = 0;
> +double width, height, h_offset, v_offset;
> +int side_data_size = 0;
> +const AVCleanAperture *clap =
> +(const AVCleanAperture *)av_stream_get_side_data(
> +st, AV_PKT_DATA_CLEAN_APERTURE, _data_size);
> +if (!clap)
> +return 0;
> +
> +width = av_q2d(clap->width);
> +height = av_q2d(clap->height);
> +h_offset = av_q2d(clap->horizontal_offset);
> +v_offset = av_q2d(clap->vertical_offset);
> +cropb = (int)(par->height - (par->height / 2. + v_offset + height / 2));
> +cropt = (int)(par->height / 2. + v_offset - height / 2);
> +cropr = (int)(par->width - (par->width / 2. + h_offset + width / 2));
> +cropl = (int)(par->width / 2. + h_offset - width / 2);
> +cropb = FFMAX(cropb, 0);
> +cropt = FFMAX(cropt, 0);
> +cropr = FFMAX(cropr, 0);
> +cropl = FFMAX(cropl, 0);
> +if (!cropr && !cropl && !cropt && !cropb)
> +return 0;
> +put_ebml_uint(pb, MATROSKA_ID_VIDEOPIXELCROPL, cropl);
> +put_ebml_uint(pb, MATROSKA_ID_VIDEOPIXELCROPR, cropr);
> +put_ebml_uint(pb, MATROSKA_ID_VIDEOPIXELCROPB, cropb);
> +put_ebml_uint(pb, MATROSKA_ID_VIDEOPIXELCROPT, cropt);
There is no reason to write all four elements when only some of them are
different from zero.
> +return 0;
> +}
> +
> static int mkv_write_video_projection(AVFormatContext *s, AVIOContext *pb,
>const AVStream *st)
> {
> @@ -1287,6 +1319,11 @@ static int mkv_write_track(AVFormatContext *s,
> MatroskaMuxContext *mkv,
> } else if (mkv->mode != MODE_WEBM)
> put_ebml_uint(pb, MATROSKA_ID_VIDEODISPLAYUNIT,
> MATROSKA_VIDEO_DISPLAYUNIT_UNKNOWN);
>
> +// Write out pixel crop
> +ret = mkv_write_video_crop(pb, par, st);
> +if (ret < 0)
> +return ret;
This is the wrong place for this: It needs to be performed before we
write the display dimensions and you need to return the cropped width
and height via pointer arguments, so that the code doing display
dimensions (which needs to be updated) knows about the cropping. This is
necessary, because cropping is supposed to happen before scaling: If the
code for writing DisplayWidth/Height didn't know about the real
dimensions to scale, it would write the DisplayWidth/Height values that
are appropriate for a video with the given pixel aspect ratio and the
uncropped pixel dimensions.
We should btw error out if someone sends stereo 3d data that implies
that a dimension is doubled if cropping is intended to be applied in the
same dimension, as this scenario
Re: [FFmpeg-devel] [PATCH] libavutil: add clean aperture (CLAP) side data.
Neil Birkbeck:
> On Tue, Apr 28, 2020 at 3:18 AM Nicolas George 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]
>
>
>
> Write out stream-level AVCleanAperture side data in the muxer.
You should separate the mov and Matroska patches.
>
> Signed-off-by: Neil Birkbeck
> ---
> libavformat/matroskaenc.c | 37 +
> libavformat/movenc.c | 28
> 2 files changed, 57 insertions(+), 8 deletions(-)
>
> diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c
> index 784973a951..19ff29853e 100644
> --- a/libavformat/matroskaenc.c
> +++ b/libavformat/matroskaenc.c
> @@ -901,6 +901,38 @@ static int mkv_write_video_color(AVIOContext *pb, const
> AVCodecParameters *par,
> return 0;
> }
>
> +static int mkv_write_video_crop(AVIOContext *pb, AVCodecParameters *par,
> AVStream *st)
> +{
> +int cropb = 0, cropt = 0, cropl = 0, cropr = 0;
> +double width, height, h_offset, v_offset;
> +int side_data_size = 0;
> +const AVCleanAperture *clap =
> +(const AVCleanAperture *)av_stream_get_side_data(
> +st, AV_PKT_DATA_CLEAN_APERTURE, _data_size);
> +if (!clap)
> +return 0;
> +
> +width = av_q2d(clap->width);
> +height = av_q2d(clap->height);
> +h_offset = av_q2d(clap->horizontal_offset);
> +v_offset = av_q2d(clap->vertical_offset);
> +cropb = (int)(par->height - (par->height / 2. + v_offset + height / 2));
> +cropt = (int)(par->height / 2. + v_offset - height / 2);
> +cropr = (int)(par->width - (par->width / 2. + h_offset + width / 2));
> +cropl = (int)(par->width / 2. + h_offset - width / 2);
> +cropb = FFMAX(cropb, 0);
> +cropt = FFMAX(cropt, 0);
> +cropr = FFMAX(cropr, 0);
> +cropl = FFMAX(cropl, 0);
> +if (!cropr && !cropl && !cropt && !cropb)
> +return 0;
> +put_ebml_uint(pb, MATROSKA_ID_VIDEOPIXELCROPL, cropl);
> +put_ebml_uint(pb, MATROSKA_ID_VIDEOPIXELCROPR, cropr);
> +put_ebml_uint(pb, MATROSKA_ID_VIDEOPIXELCROPB, cropb);
> +put_ebml_uint(pb, MATROSKA_ID_VIDEOPIXELCROPT, cropt);
There is no reason to write all four elements when only some of them are
different from zero.
> +return 0;
> +}
> +
> static int mkv_write_video_projection(AVFormatContext *s, AVIOContext *pb,
>const AVStream *st)
> {
> @@ -1287,6 +1319,11 @@ static int mkv_write_track(AVFormatContext *s,
> MatroskaMuxContext *mkv,
> } else if (mkv->mode != MODE_WEBM)
> put_ebml_uint(pb, MATROSKA_ID_VIDEODISPLAYUNIT,
> MATROSKA_VIDEO_DISPLAYUNIT_UNKNOWN);
>
> +// Write out pixel crop
> +ret = mkv_write_video_crop(pb, par, st);
> +if (ret < 0)
> +return ret;
This is the wrong place for this: It needs to be performed before we
write the display dimensions and you need to return the cropped width
and height via pointer arguments, so that the code doing display
dimensions (which needs to be updated) knows about the cropping. This is
necessary, because cropping is supposed to happen before scaling: If the
code for writing DisplayWidth/Height didn't know about the real
dimensions to scale, it would write the DisplayWidth/Height values that
are appropriate for a video with the given pixel aspect ratio and the
uncropped pixel dimensions.
We should btw error out if someone sends stereo 3d data that implies
that a dimension is doubled if cropping is intended to be applied in the
same dimension, as this scenario
Re: [FFmpeg-devel] [PATCH] libavutil: add clean aperture (CLAP) side data.
On 5/6/2020 12:22 PM, Neil Birkbeck wrote:
> On Tue, May 5, 2020 at 5:11 AM Kieran O Leary
> 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
>> wrote:
>>
>>> On Tue, Apr 28, 2020 at 3:18 AM Nicolas George 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 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.
___
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".
Re: [FFmpeg-devel] [PATCH] libavutil: add clean aperture (CLAP) side data.
On Tue, May 5, 2020 at 5:11 AM Kieran O Leary
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
> wrote:
>
> > On Tue, Apr 28, 2020 at 3:18 AM Nicolas George 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.
___
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".
Re: [FFmpeg-devel] [PATCH] libavutil: add clean aperture (CLAP) side data.
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
wrote:
> On Tue, Apr 28, 2020 at 3:18 AM Nicolas George 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:
$ ./ffmpeg -i /mnt/c/Users/blaaa/Downloads/clap.mov -c:v v210 newv210.mov
&& mediainfo --Details=1 newv210.mov |grep -i clap -n10
ffmpeg version N-97506-g2fae000994 Copyright (c) 2000-2020 the FFmpeg
developers
built with gcc 7 (Ubuntu 7.5.0-3ubuntu1~18.04)
configuration:
libavutil 56. 43.100 / 56. 43.100
libavcodec 58. 82.100 / 58. 82.100
libavformat58. 42.101 / 58. 42.101
libavdevice58. 9.103 / 58. 9.103
libavfilter 7. 79.100 / 7. 79.100
libswscale 5. 6.101 / 5. 6.101
libswresample 3. 6.100 / 3. 6.100
Guessed Channel Layout for Input Stream #0.1 : stereo
Input #0, mov,mp4,m4a,3gp,3g2,mj2, from
'/mnt/c/Users/blaaa/Downloads/clap.mov':
Metadata:
major_brand : qt
minor_version : 537199360
compatible_brands: qt
creation_time : 2018-09-13T08:48:41.00Z
Duration: 00:00:01.00, start: 0.00, bitrate: 80686 kb/s
Stream #0:0(eng): Video: v210 (v210 / 0x30313276),
yuv422p10le(smpte170m/bt470bg/bt709, top coded first (swapped)), 720x576,
79626 kb/s, SAR 59:54 DAR 295:216, 9 fps, 25 tbr, 25k tbn, 25k tbc (default)
Metadata:
creation_time : 2018-09-13T08:48:41.00Z
handler_name: ?Apple Video Media Handler
encoder : Uncompressed 10-Bit YUV
timecode: 00:00:00:00
Side data:
Clean aperture:[width 41472/59 height:576/1 h_offset:0/1 v_offset:0/1]
Stream #0:1(eng): Audio: pcm_s24le (in24 / 0x34326E69), 48000 Hz,
stereo, s32 (24 bit), 2304 kb/s (default)
Metadata:
creation_time : 2018-09-13T08:48:41.00Z
handler_name: ?Apple Sound Media Handler
timecode: 00:00:00:00
Stream #0:2(eng): Data: none (tmcd / 0x64636D74), 0 kb/s (default)
Metadata:
creation_time : 2018-09-13T08:48:41.00Z
handler_name: Time Code Media Handler
reel_name : 001
timecode: 00:00:00:00
File 'newv210.mov' already exists. Overwrite? [y/N] y
Stream mapping:
Stream #0:0 -> #0:0 (v210 (native) -> v210 (native))
Stream #0:1 -> #0:1 (pcm_s24le (native) -> aac (native))
Press [q] to stop, [?] for help
Output #0, mov, to 'newv210.mov':
Metadata:
major_brand : qt
minor_version : 537199360
compatible_brands: qt
encoder : Lavf58.42.101
Stream #0:0(eng): Video: v210 (v210 / 0x30313276), yuv422p10le(top
coded first (swapped)), 720x576 [SAR 59:54 DAR 295:216], q=2-31, 221184
kb/s, 0.04 fps, 12800 tbn, 25 tbc (default)
Metadata:
creation_time : 2018-09-13T08:48:41.00Z
handler_name: ?Apple Video Media Handler
timecode: 00:00:00:00
encoder : Lavc58.82.100 v210
Side data:
Clean aperture:[width 41472/59 height:576/1 h_offset:0/1 v_offset:0/1]
Stream #0:1(eng): Audio: aac (LC) (mp4a / 0x6134706D), 48000 Hz,
stereo, fltp (24 bit), 128 kb/s (default)
Metadata:
Re: [FFmpeg-devel] [PATCH] libavutil: add clean aperture (CLAP) side data.
Hi Neil, 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: $ ./ffmpeg -i /mnt/c/Users/blaaa/Downloads/clap.mov -c:v v210 newv210.mov && mediainfo --Details=1 newv210.mov |grep -i clap -n10 ffmpeg version N-97506-g2fae000994 Copyright (c) 2000-2020 the FFmpeg developers built with gcc 7 (Ubuntu 7.5.0-3ubuntu1~18.04) configuration: libavutil 56. 43.100 / 56. 43.100 libavcodec 58. 82.100 / 58. 82.100 libavformat58. 42.101 / 58. 42.101 libavdevice58. 9.103 / 58. 9.103 libavfilter 7. 79.100 / 7. 79.100 libswscale 5. 6.101 / 5. 6.101 libswresample 3. 6.100 / 3. 6.100 Guessed Channel Layout for Input Stream #0.1 : stereo Input #0, mov,mp4,m4a,3gp,3g2,mj2, from '/mnt/c/Users/blaaa/Downloads/clap.mov': Metadata: major_brand : qt minor_version : 537199360 compatible_brands: qt creation_time : 2018-09-13T08:48:41.00Z Duration: 00:00:01.00, start: 0.00, bitrate: 80686 kb/s Stream #0:0(eng): Video: v210 (v210 / 0x30313276), yuv422p10le(smpte170m/bt470bg/bt709, top coded first (swapped)), 720x576, 79626 kb/s, SAR 59:54 DAR 295:216, 9 fps, 25 tbr, 25k tbn, 25k tbc (default) Metadata: creation_time : 2018-09-13T08:48:41.00Z handler_name: ?Apple Video Media Handler encoder : Uncompressed 10-Bit YUV timecode: 00:00:00:00 Side data: Clean aperture:[width 41472/59 height:576/1 h_offset:0/1 v_offset:0/1] Stream #0:1(eng): Audio: pcm_s24le (in24 / 0x34326E69), 48000 Hz, stereo, s32 (24 bit), 2304 kb/s (default) Metadata: creation_time : 2018-09-13T08:48:41.00Z handler_name: ?Apple Sound Media Handler timecode: 00:00:00:00 Stream #0:2(eng): Data: none (tmcd / 0x64636D74), 0 kb/s (default) Metadata: creation_time : 2018-09-13T08:48:41.00Z handler_name: Time Code Media Handler reel_name : 001 timecode: 00:00:00:00 File 'newv210.mov' already exists. Overwrite? [y/N] y Stream mapping: Stream #0:0 -> #0:0 (v210 (native) -> v210 (native)) Stream #0:1 -> #0:1 (pcm_s24le (native) -> aac (native)) Press [q] to stop, [?] for help Output #0, mov, to 'newv210.mov': Metadata: major_brand : qt minor_version : 537199360 compatible_brands: qt encoder : Lavf58.42.101 Stream #0:0(eng): Video: v210 (v210 / 0x30313276), yuv422p10le(top coded first (swapped)), 720x576 [SAR 59:54 DAR 295:216], q=2-31, 221184 kb/s, 0.04 fps, 12800 tbn, 25 tbc (default) Metadata: creation_time : 2018-09-13T08:48:41.00Z handler_name: ?Apple Video Media Handler timecode: 00:00:00:00 encoder : Lavc58.82.100 v210 Side data: Clean aperture:[width 41472/59 height:576/1 h_offset:0/1 v_offset:0/1] Stream #0:1(eng): Audio: aac (LC) (mp4a / 0x6134706D), 48000 Hz, stereo, fltp (24 bit), 128 kb/s (default) Metadata: creation_time : 2018-09-13T08:48:41.00Z handler_name: ?Apple Sound Media Handler timecode: 00:00:00:00 encoder : Lavc58.82.100 aac frame= 25 fps=0.0 q=-0.0 Lsize= 27009kB time=00:00:00.96 bitrate=230455.2kbits/s dup=16 drop=0 speed=19.8x video:27000kB audio:6kB subtitle:0kB other streams:0kB global headers:0kB muxing overhead: 0.008794% [aac @ 0x7fffe91cc680] Qavg: 171.537 226-1A5FBA4 detail: 9 (0x09) 227-1A5FBA5Pixel Aspect Ratio (16 bytes) 228-1A5FBA5 Header (8 bytes) 229-1A5FBA5 Size: 16 (0x0010) 230-1A5FBA9 Name: pasp 231-1A5FBAD hSpacing: 59 (0x003B) 232-1A5FBB1 vSpacing: 54 (0x0036) 233-1A5FBB5Clean Aperture (40 bytes) 234-1A5FBB5 Header (8 bytes) 235-1A5FBB5 Size: 40 (0x0028) 236:1A5FBB9 Name: clap 237-1A5FBBD apertureWidth_N:41472 (0xA200) 238-1A5FBC1 apertureWidth_D:59 (0x003B) 239-1A5FBC5 apertureHeight_N: 576 (0x0240) 240-1A5FBC9 apertureHeight_D: 1 (0x0001) 241-1A5FBCD horizOff_N: 0 (0x) 242-1A5FBD1 horizOff_D: 1 (0x0001) 243-1A5FBD5 vertOff_N: 0 (0x) 244-1A5FBD9 vertOff_D: 1 (0x0001) 245-1A5FBDD Time to Sample (24 bytes) 246-1A5FBDD Header (8 bytes) and here's similar info for mkv: ./ffmpeg -i
Re: [FFmpeg-devel] [PATCH] libavutil: add clean aperture (CLAP) side data.
On Tue, Apr 28, 2020 at 3:18 AM Nicolas George 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]
From d92371cc2de6a671b44d9f6dd9d28544d05b5287 Mon Sep 17 00:00:00 2001
From: Neil Birkbeck
Date: Sun, 26 Apr 2020 20:42:59 -0700
Subject: [PATCH 3/3] libavformat: Adding MOV/MKV muxer support for CLAP side
data.
Write out stream-level AVCleanAperture side data in the muxer.
Signed-off-by: Neil Birkbeck
---
libavformat/matroskaenc.c | 37 +
libavformat/movenc.c | 28
2 files changed, 57 insertions(+), 8 deletions(-)
diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c
index 784973a951..19ff29853e 100644
--- a/libavformat/matroskaenc.c
+++ b/libavformat/matroskaenc.c
@@ -901,6 +901,38 @@ static int mkv_write_video_color(AVIOContext *pb, const AVCodecParameters *par,
return 0;
}
+static int mkv_write_video_crop(AVIOContext *pb, AVCodecParameters *par, AVStream *st)
+{
+int cropb = 0, cropt = 0, cropl = 0, cropr = 0;
+double width, height, h_offset, v_offset;
+int side_data_size = 0;
+const AVCleanAperture *clap =
+(const AVCleanAperture *)av_stream_get_side_data(
+st, AV_PKT_DATA_CLEAN_APERTURE, _data_size);
+if (!clap)
+return 0;
+
+width = av_q2d(clap->width);
+height = av_q2d(clap->height);
+h_offset = av_q2d(clap->horizontal_offset);
+v_offset = av_q2d(clap->vertical_offset);
+cropb = (int)(par->height - (par->height / 2. + v_offset + height / 2));
+cropt = (int)(par->height / 2. + v_offset - height / 2);
+cropr = (int)(par->width - (par->width / 2. + h_offset + width / 2));
+cropl = (int)(par->width / 2. + h_offset - width / 2);
+cropb = FFMAX(cropb, 0);
+cropt = FFMAX(cropt, 0);
+cropr = FFMAX(cropr, 0);
+cropl = FFMAX(cropl, 0);
+if (!cropr && !cropl && !cropt && !cropb)
+return 0;
+put_ebml_uint(pb, MATROSKA_ID_VIDEOPIXELCROPL, cropl);
+put_ebml_uint(pb, MATROSKA_ID_VIDEOPIXELCROPR, cropr);
+put_ebml_uint(pb, MATROSKA_ID_VIDEOPIXELCROPB, cropb);
+put_ebml_uint(pb, MATROSKA_ID_VIDEOPIXELCROPT, cropt);
+return 0;
+}
+
static int mkv_write_video_projection(AVFormatContext *s, AVIOContext *pb,
const AVStream *st)
{
@@ -1287,6 +1319,11 @@ static int mkv_write_track(AVFormatContext *s, MatroskaMuxContext *mkv,
} else if (mkv->mode != MODE_WEBM)
put_ebml_uint(pb, MATROSKA_ID_VIDEODISPLAYUNIT, MATROSKA_VIDEO_DISPLAYUNIT_UNKNOWN);
+// Write out pixel crop
+ret = mkv_write_video_crop(pb, par, st);
+if (ret < 0)
+return ret;
+
if (par->codec_id == AV_CODEC_ID_RAWVIDEO) {
uint32_t color_space = av_le2ne32(par->codec_tag);
put_ebml_binary(pb, MATROSKA_ID_VIDEOCOLORSPACE, _space, sizeof(color_space));
diff --git a/libavformat/movenc.c b/libavformat/movenc.c
index 32e8109268..ab2b53cab6 100644
--- a/libavformat/movenc.c
+++ b/libavformat/movenc.c
@@ -1847,16 +1847,28 @@ static int mov_write_dvcc_dvvc_tag(AVFormatContext *s, AVIOContext *pb, AVDOVIDe
static int mov_write_clap_tag(AVIOContext *pb, MOVTrack *track)
{
+const AVCleanAperture* clap =
+(const AVCleanAperture*) av_stream_get_side_data(
+track->st, AV_PKT_DATA_CLEAN_APERTURE, NULL);
+AVRational width = {.num = clap ? clap->width.num : track->par->width,
+.den = clap ? clap->width.den : 1};
+AVRational height = {.num = clap ? clap->height.num :
Re: [FFmpeg-devel] [PATCH] libavutil: add clean aperture (CLAP) side data.
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. Regards, -- Nicolas George signature.asc Description: PGP signature ___ 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".
Re: [FFmpeg-devel] [PATCH] libavutil: add clean aperture (CLAP) side data.
Kieran O Leary: > Hi Neil, > > Thanks so much for working on this - what was the impetus? > I started the ticket you mentioned. I applied your patch and tested it with > the clap.mov file from that ticket. How do I know if behaviour has changed > with this patch, how should I be testing? > I don't see any reference to the clap atom values when transcoding the file > to MKV or to another mov: > 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). - Andreas ___ 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".
Re: [FFmpeg-devel] [PATCH] libavutil: add clean aperture (CLAP) side data.
Hi Neil, Thanks so much for working on this - what was the impetus? I started the ticket you mentioned. I applied your patch and tested it with the clap.mov file from that ticket. How do I know if behaviour has changed with this patch, how should I be testing? I don't see any reference to the clap atom values when transcoding the file to MKV or to another mov: ./ffmpeg -i /mnt/c/Users/kieran.oleary/Downloads/clap.mov out.mkv ffmpeg version N-97506-g2fae000994 Copyright (c) 2000-2020 the FFmpeg developers built with gcc 7 (Ubuntu 7.5.0-3ubuntu1~18.04) configuration: libavutil 56. 43.100 / 56. 43.100 libavcodec 58. 82.100 / 58. 82.100 libavformat58. 42.101 / 58. 42.101 libavdevice58. 9.103 / 58. 9.103 libavfilter 7. 79.100 / 7. 79.100 libswscale 5. 6.101 / 5. 6.101 libswresample 3. 6.100 / 3. 6.100 Guessed Channel Layout for Input Stream #0.1 : stereo Input #0, mov,mp4,m4a,3gp,3g2,mj2, from '/mnt/c/Users/kieran.oleary/Downloads/clap.mov': Metadata: major_brand : qt minor_version : 537199360 compatible_brands: qt creation_time : 2018-09-13T08:48:41.00Z Duration: 00:00:01.00, start: 0.00, bitrate: 80686 kb/s Stream #0:0(eng): Video: v210 (v210 / 0x30313276), yuv422p10le(smpte170m/bt470bg/bt709, top coded first (swapped)), 720x576, 79626 kb/s, SAR 59:54 DAR 295:216, 9 fps, 25 tbr, 25k tbn, 25k tbc (default) Metadata: creation_time : 2018-09-13T08:48:41.00Z handler_name: ?Apple Video Media Handler encoder : Uncompressed 10-Bit YUV timecode: 00:00:00:00 Stream #0:1(eng): Audio: pcm_s24le (in24 / 0x34326E69), 48000 Hz, stereo, s32 (24 bit), 2304 kb/s (default) Metadata: creation_time : 2018-09-13T08:48:41.00Z handler_name: ?Apple Sound Media Handler timecode: 00:00:00:00 Stream #0:2(eng): Data: none (tmcd / 0x64636D74), 0 kb/s (default) Metadata: creation_time : 2018-09-13T08:48:41.00Z handler_name: Time Code Media Handler reel_name : 001 timecode: 00:00:00:00 File 'out.mkv' already exists. Overwrite? [y/N] y Stream mapping: Stream #0:0 -> #0:0 (v210 (native) -> mpeg4 (native)) Stream #0:1 -> #0:1 (pcm_s24le (native) -> ac3 (native)) Press [q] to stop, [?] for help Output #0, matroska, to 'out.mkv': Metadata: major_brand : qt minor_version : 537199360 compatible_brands: qt encoder : Lavf58.42.101 Stream #0:0(eng): Video: mpeg4 (FMP4 / 0x34504D46), yuv420p(top coded first (swapped)), 720x576 [SAR 59:54 DAR 295:216], q=2-31, 200 kb/s, 25 fps, 1k tbn, 25 tbc (default) Metadata: creation_time : 2018-09-13T08:48:41.00Z handler_name: ?Apple Video Media Handler timecode: 00:00:00:00 encoder : Lavc58.82.100 mpeg4 Side data: cpb: bitrate max/min/avg: 0/0/20 buffer size: 0 vbv_delay: N/A Stream #0:1(eng): Audio: ac3 ([0] [0][0] / 0x2000), 48000 Hz, stereo, fltp (24 bit), 192 kb/s (default) Metadata: creation_time : 2018-09-13T08:48:41.00Z handler_name: ?Apple Sound Media Handler timecode: 00:00:00:00 encoder : Lavc58.82.100 ac3 frame=9 fps=0.0 q=1.6 Lsize= 36kB time=00:00:00.41 bitrate= 717.1kbits/s speed=9.05x video:25kB audio:10kB subtitle:0kB other streams:0kB global headers:0kB muxing overhead: 3.596637% ___ 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".
Re: [FFmpeg-devel] [PATCH] libavutil: add clean aperture (CLAP) side data.
On Mon, Apr 27, 2020 at 4:38 AM Lynne wrote: > Apr 27, 2020, 05:27 by neil.birkb...@gmail.com: > > > The clean aperature represents a cropping of the stored image data used > to > > relate the image data to a canonical video system and exists as container > > metadata (see 'clap' section in > https://developer.apple.com/library/archive/documentation/QuickTime/QTFF/QTFFChap3/qtff3.html > ) > > > > Addition of the side data is a first step towards demuxing CLAP atom > metadata, > > helping to resolve https://trac.ffmpeg.org/ticket/7437 > > > > This CleanAperture representation can also carry PixelCrop fields from > MKV. > > Side data was suggested as a way to carry such PixelCrop fields in: > > https://ffmpeg.org/pipermail/ffmpeg-devel/2016-March/192302.html > > > > Transmuxing the side data can then be added (MOV to/from MKV), and > > auto-application could optionally be enabled like autorotate in > ffmpeg_filter.c. > > > > We already have frame cropping metadata in the AVFrame structure. > Propagating the crop fields from the format/container to AVFrame was my initial thought. As far as I can tell, currently the cropping fields on AVFrame are typically set from the coded bitstream. And plumbing from the container requires the crop fields to be added to AVCodecParameters and to AVCodecContext, which could then be set on the AVFrame in decode.c. In part, I based the SideData on some older threads on the topic, but those threads predate the AVFrame-level cropping. I still see a few a couple of other potential reasons: -For the transmux use cases, I was concerned adding the extra fields within AVCodecParameters/AVCodecContext may confuse the interfaces as to whether crop/CLAP was destined for the container or the codec. As stream metadata, it seemed more clear that it should belong in the container -And a minor one: for the CLAP atom case, on remuxing, the existing integer crop fields could cause some small sub-pixel loss. Is it preferred to add the extra fields in AVCodecParameters/AVCodecContext and plumb them into the frame instead? That should work for the case that I'm trying to resolve (applying PixelCrop from MKV), but wanted to make sure the other use cases (e.g., transmux, CLAP atom) worked too. ___ 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".
Re: [FFmpeg-devel] [PATCH] libavutil: add clean aperture (CLAP) side data.
Apr 27, 2020, 05:27 by neil.birkb...@gmail.com: > The clean aperature represents a cropping of the stored image data used to > relate the image data to a canonical video system and exists as container > metadata (see 'clap' section in > https://developer.apple.com/library/archive/documentation/QuickTime/QTFF/QTFFChap3/qtff3.html) > > Addition of the side data is a first step towards demuxing CLAP atom metadata, > helping to resolve https://trac.ffmpeg.org/ticket/7437 > > This CleanAperture representation can also carry PixelCrop fields from MKV. > Side data was suggested as a way to carry such PixelCrop fields in: > https://ffmpeg.org/pipermail/ffmpeg-devel/2016-March/192302.html > > Transmuxing the side data can then be added (MOV to/from MKV), and > auto-application could optionally be enabled like autorotate in > ffmpeg_filter.c. > We already have frame cropping metadata in the AVFrame structure. ___ 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".
