[FFmpeg-devel] movenc: add write_btrt option

[Jan Ekström]

On Thu, Apr 7, 2022 at 11:42 AM Eran Kornblau <eran.kornblau at kaltura.com> wrote:
>
> >
> >
> > -----Original Message-----
> > From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> On Behalf Of "zhilizhao(???)"
> > Sent: Wednesday, 6 April 2022 11:46
> > To: FFmpeg development discussions and patches <ffmpeg-devel at ffmpeg.org>
> > Subject: Re: [FFmpeg-devel] movenc: add write_btrt option
> >
> > > On Apr 3, 2022, at 1:07 PM, Eran Kornblau <eran.kornblau at kaltura.com> wrote:
> > >
> > > Trying my luck in a new thread…
> > >
> > > This patch is in continuation to this discussion –
> > > https://eur02.safelinks.protection.outlook.com/?url=https%3A%2F%2Fffmp
> > > eg.org%2Fpipermail%2Fffmpeg-devel%2F2022-March%2F294623.html&data=
> > > 04%7C01%7C%7Cb9907958f97048f5645708da17a9f3c8%7C0c503748de3f4e2597e268
> > > 19d53a42b6%7C1%7C0%7C637848315958196733%7CUnknown%7CTWFpbGZsb3d8eyJWIj
> > > oiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&am
> > > p;sdata=flQc21b5EVWTy%2Bkmw%2FncIWzdLqUxY5XFislPRs5Ij6o%3D&reserve
> > > d=0
> > >
> > > supports forcing or disabling the writing of the btrt atom.
> > > the default behavior is to write the atom only for mp4 mode.
> > > ---
> > >  libavformat/movenc.c | 30 +++++++++++++++++++-----------
> > > libavformat/movenc.h |  1 +
> > >  2 files changed, 20 insertions(+), 11 deletions(-)
> > >
> > > diff --git a/libavformat/movenc.c b/libavformat/movenc.c index
> > > 4c868919ae..b75f1c6909 100644
> > > --- a/libavformat/movenc.c
> > > +++ b/libavformat/movenc.c
> > >
> > […]
> > >
> > > -    if (track->mode == MODE_MP4 &&
> > > -            ((ret = mov_write_btrt_tag(pb, track)) < 0))
> > > -        return ret;
> > > +    if ((mov->write_btrt == -1 && track->mode == MODE_MP4) || mov->write_btrt == 1) {
> > > +        if ((ret = mov_write_btrt_tag(pb, track)) < 0) {
> > > +            return ret;
> > > +        }
> > > +    }
> >
> > I prefer to handle the auto mode (mov->write_btrt == -1) in a single place, so we don’t need to change multiple lines if the condition changed, e.g., enable btrt for MODE_MOV. Please correct me if I’m wrong, mov_init() has all of the contexts to overwrite mov->write_btrt.
> >
> Makes sense, thanks for the feedback!
> Updated patch attached
>
> Eran

Generally speaking I am not against this patch. Would have possibly
came up with something similar myself in case the actual output of
libavformat would have caused issues, which surprisingly enough it
wasn't.

I know you have just copied the logic from tmcd or so, but I think the
-1 logic is unnecessary. We don't need to force writing of this
structure no matter what, so you either have it enabled (by default),
or disabled. If additional formats such as QTFF have since added the
btrt box into their specification, that doesn't need forcing, but
rather addition of it into the logic later (if you wanted forcing then
you'd have to deal with strict_std_compliance being
unofficial/experimental or higher etc, and if this was not set -
warning the user that a not officially defined functionality was being
written into the container and exiting with AVERROR_EXPERIMENTAL).

Additionally, I thought new options go to the end of the AVOption
array, but then saw 1dddb930aaf0cadaa19f86e81225c9c352745262 where
James added "crf" into the middle of an array... so I guess since it's
an array and not a struct the location no longer matters as much?
┐(´д｀)┌ Although the struct integer should definitely go to the end of
it, otherwise you are breaking existing offsets? Although thankfully,
the struct isn't externally exposed so someone else could chime in
regarding this, I am unfortunately quite tired throughout this week :P
.

Jan