[FFmpeg-devel] movenc: add write_btrt option

[FFmpeg-devel] movenc: add write_btrt option

[Eran Kornblau]

[-- Attachment #1: Type: text/plain, Size: 178 bytes --]

Trying my luck in a new thread…

This patch is in continuation to this discussion –
https://ffmpeg.org/pipermail/ffmpeg-devel/2022-March/294623.html

Thanks!

Eran


[-- Attachment #2: 0001-movenc-add-write_btrt-option.patch --]
[-- Type: application/octet-stream, Size: 4913 bytes --]

[-- Attachment #3: Type: text/plain, Size: 251 bytes --]

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] movenc: add write_btrt option

["zhilizhao(赵志立)"]

> On Apr 3, 2022, at 1:07 PM, Eran Kornblau <eran.kornblau@kaltura.com> wrote:
> 
> Trying my luck in a new thread…
> 
> This patch is in continuation to this discussion –
> https://ffmpeg.org/pipermail/ffmpeg-devel/2022-March/294623.html
> 
> 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.
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] movenc: add write_btrt option

[Eran Kornblau]

[-- Attachment #1: Type: text/plain, Size: 2152 bytes --]

> 
> 
> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of "zhilizhao(???)"
> Sent: Wednesday, 6 April 2022 11:46
> To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>
> Subject: Re: [FFmpeg-devel] movenc: add write_btrt option
> 
> > On Apr 3, 2022, at 1:07 PM, Eran Kornblau <eran.kornblau@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&amp;data=
> > 04%7C01%7C%7Cb9907958f97048f5645708da17a9f3c8%7C0c503748de3f4e2597e268
> > 19d53a42b6%7C1%7C0%7C637848315958196733%7CUnknown%7CTWFpbGZsb3d8eyJWIj
> > oiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&am
> > p;sdata=flQc21b5EVWTy%2Bkmw%2FncIWzdLqUxY5XFislPRs5Ij6o%3D&amp;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

[-- Attachment #2: 0001-movenc-add-write_btrt-option.patch --]
[-- Type: application/octet-stream, Size: 4544 bytes --]

[-- Attachment #3: Type: text/plain, Size: 251 bytes --]

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] movenc: add write_btrt option

["zhilizhao(赵志立)"]

> On Apr 7, 2022, at 4:42 PM, Eran Kornblau <eran.kornblau@kaltura.com> wrote:
> 
>> 
>> 
>> -----Original Message-----
>> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of "zhilizhao(???)"
>> Sent: Wednesday, 6 April 2022 11:46
>> To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>
>> Subject: Re: [FFmpeg-devel] movenc: add write_btrt option
>>> 
>>> 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(-)
>>> 
>> 
>> 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

LGTM.

> 
> Eran
> <0001-movenc-add-write_btrt-option.patch>_______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
> 

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] movenc: add write_btrt option

[Jan Ekström]

On Thu, Apr 7, 2022 at 11:42 AM Eran Kornblau <eran.kornblau@kaltura.com> wrote:
>
> >
> >
> > -----Original Message-----
> > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of "zhilizhao(???)"
> > Sent: Wednesday, 6 April 2022 11:46
> > To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>
> > Subject: Re: [FFmpeg-devel] movenc: add write_btrt option
> >
> > > On Apr 3, 2022, at 1:07 PM, Eran Kornblau <eran.kornblau@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&amp;data=
> > > 04%7C01%7C%7Cb9907958f97048f5645708da17a9f3c8%7C0c503748de3f4e2597e268
> > > 19d53a42b6%7C1%7C0%7C637848315958196733%7CUnknown%7CTWFpbGZsb3d8eyJWIj
> > > oiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&am
> > > p;sdata=flQc21b5EVWTy%2Bkmw%2FncIWzdLqUxY5XFislPRs5Ij6o%3D&amp;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
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] movenc: add write_btrt option

["zhilizhao(赵志立)"]

> On Apr 8, 2022, at 4:36 AM, Jan Ekström <jeebjp@gmail.com> wrote:
> 
> On Thu, Apr 7, 2022 at 11:42 AM Eran Kornblau <eran.kornblau@kaltura.com> wrote:
>> 
>>> 
>>> 
>>> -----Original Message-----
>>> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of "zhilizhao(???)"
>>> Sent: Wednesday, 6 April 2022 11:46
>>> To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>
>>> Subject: Re: [FFmpeg-devel] movenc: add write_btrt option
>>> 
>>>> On Apr 3, 2022, at 1:07 PM, Eran Kornblau <eran.kornblau@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&amp;data=
>>>> 04%7C01%7C%7Cb9907958f97048f5645708da17a9f3c8%7C0c503748de3f4e2597e268
>>>> 19d53a42b6%7C1%7C0%7C637848315958196733%7CUnknown%7CTWFpbGZsb3d8eyJWIj
>>>> oiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&am
>>>> p;sdata=flQc21b5EVWTy%2Bkmw%2FncIWzdLqUxY5XFislPRs5Ij6o%3D&amp;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
> .

The order of options and the offset of fields in private struct have no
effect on ABI. I take these into consideration:
1. Readability. Related options and fields should be put at the same
   place.
2. Memory footprint. Reduce struct padding.

> 
> Jan
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] movenc: add write_btrt option

[Jan Ekström]

On Fri, Apr 8, 2022 at 5:47 AM "zhilizhao(赵志立)" <quinkblack@foxmail.com> wrote:
>
>
>
> > On Apr 8, 2022, at 4:36 AM, Jan Ekström <jeebjp@gmail.com> wrote:
> >
> > On Thu, Apr 7, 2022 at 11:42 AM Eran Kornblau <eran.kornblau@kaltura.com> wrote:
> >>
> >>>
> >>>
> >>> -----Original Message-----
> >>> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of "zhilizhao(???)"
> >>> Sent: Wednesday, 6 April 2022 11:46
> >>> To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>
> >>> Subject: Re: [FFmpeg-devel] movenc: add write_btrt option
> >>>
> >>>> On Apr 3, 2022, at 1:07 PM, Eran Kornblau <eran.kornblau@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&amp;data=
> >>>> 04%7C01%7C%7Cb9907958f97048f5645708da17a9f3c8%7C0c503748de3f4e2597e268
> >>>> 19d53a42b6%7C1%7C0%7C637848315958196733%7CUnknown%7CTWFpbGZsb3d8eyJWIj
> >>>> oiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&am
> >>>> p;sdata=flQc21b5EVWTy%2Bkmw%2FncIWzdLqUxY5XFislPRs5Ij6o%3D&amp;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
> > .
>
> The order of options and the offset of fields in private struct have no
> effect on ABI. I take these into consideration:
> 1. Readability. Related options and fields should be put at the same
>    place.
> 2. Memory footprint. Reduce struct padding.
>

Yes, this is a minor thing within my comment, my comment was mostly
regarding the -1 case being unnecessary (since I don't think we need
to actually force-force this, just controlling whether this box is
written or not under the general rules of where it is defined).

And yes, if the order doesn't matter then grouping makes sense. It's
just that for very long "add to the end" was the general principle, so
I was mostly utilizing this as a base to request comments from others
regarding this.

Jan
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] movenc: add write_btrt option

[Eran Kornblau]

> 
> On Fri, Apr 8, 2022 at 5:47 AM "zhilizhao(赵志立)" <quinkblack@foxmail.com> wrote:
> >
> > > On Apr 8, 2022, at 4:36 AM, Jan Ekström <jeebjp@gmail.com> wrote:
> > >
> > > On Thu, Apr 7, 2022 at 11:42 AM Eran Kornblau <eran.kornblau@kaltura.com> wrote:
> > >>
> > >>>
> > >>> -----Original Message-----
> > >>> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of "zhilizhao(???)"
> > >>> Sent: Wednesday, 6 April 2022 11:46
> > >>> To: FFmpeg development discussions and patches 
> > >>> <ffmpeg-devel@ffmpeg.org>
> > >>> Subject: Re: [FFmpeg-devel] movenc: add write_btrt option
> > >>>
> > >>>> On Apr 3, 2022, at 1:07 PM, Eran Kornblau <eran.kornblau@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%2
> > >>>> Fffmp 
> > >>>> eg.org%2Fpipermail%2Fffmpeg-devel%2F2022-March%2F294623.html&amp;
> > >>>> data=
> > >>>> 04%7C01%7C%7Cb9907958f97048f5645708da17a9f3c8%7C0c503748de3f4e259
> > >>>> 7e268 
> > >>>> 19d53a42b6%7C1%7C0%7C637848315958196733%7CUnknown%7CTWFpbGZsb3d8e
> > >>>> yJWIj 
> > >>>> oiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C30
> > >>>> 00&am 
> > >>>> p;sdata=flQc21b5EVWTy%2Bkmw%2FncIWzdLqUxY5XFislPRs5Ij6o%3D&amp;re
> > >>>> serve
> > >>>> 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 .
> >
> > The order of options and the offset of fields in private struct have 
> > no effect on ABI. I take these into consideration:
> > 1. Readability. Related options and fields should be put at the same
> >    place.
> > 2. Memory footprint. Reduce struct padding.
> >
> 
> Yes, this is a minor thing within my comment, my comment was mostly regarding the -1 case being unnecessary (since I don't think we need to actually force-force this, just controlling whether this box is written or not under the general rules of where it is defined).
> 
> And yes, if the order doesn't matter then grouping makes sense. It's just that for very long "add to the end" was the general principle, so I was mostly utilizing this as a base to request comments from others regarding this.
> 
> Jan

About the order, I agree with what Zhao wrote - I preferred to put it near write_tmcd/write_prft since they are similar.
I don't mind moving to the end, if that is the decision.

Regarding the ability to force it, personally, I think that in this case, supporting force doesn't add any complexity to the code,
and maybe someone will find it useful at some point. But again, if there is strong objection to this, I will submit a patch without it.

Thanks,

Eran

> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://eur02.safelinks.protection.outlook.com/?url=https%3A%2F%2Fffmpeg.org%2Fmailman%2Flistinfo%2Fffmpeg-devel&amp;data=04%7C01%7C%7C2dc30bb562ec48b1939b08da194d5541%7C0c503748de3f4e2597e26819d53a42b6%7C1%7C1%7C637850117181491627%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=YGNFjOd3rLBnswLNfx0YwIOLx%2BXGW6kiL73KfdvHl9I%3D&amp;reserved=0
> 
> To unsubscribe, visit link above, or email ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] movenc: add write_btrt option

[Eran Kornblau]

> 
> > 
> > On Fri, Apr 8, 2022 at 5:47 AM "zhilizhao(赵志立)" <quinkblack@foxmail.com> wrote:
> > >
> > > > On Apr 8, 2022, at 4:36 AM, Jan Ekström <jeebjp@gmail.com> wrote:
> > > >
> > > > On Thu, Apr 7, 2022 at 11:42 AM Eran Kornblau <eran.kornblau@kaltura.com> wrote:
> > > >>
> > > >>>
> > > >>> -----Original Message-----
> > > >>> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of "zhilizhao(???)"
> > > >>> Sent: Wednesday, 6 April 2022 11:46
> > > >>> To: FFmpeg development discussions and patches 
> > > >>> <ffmpeg-devel@ffmpeg.org>
> > > >>> Subject: Re: [FFmpeg-devel] movenc: add write_btrt option
> > > >>>
> > > >>>> On Apr 3, 2022, at 1:07 PM, Eran Kornblau <eran.kornblau@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
> > > >>>> %2
> > > >>>> Fffmp
> > > >>>> eg.org%2Fpipermail%2Fffmpeg-devel%2F2022-March%2F294623.html&am
> > > >>>> p;
> > > >>>> data=
> > > >>>> 04%7C01%7C%7Cb9907958f97048f5645708da17a9f3c8%7C0c503748de3f4e2
> > > >>>> 59
> > > >>>> 7e268
> > > >>>> 19d53a42b6%7C1%7C0%7C637848315958196733%7CUnknown%7CTWFpbGZsb3d
> > > >>>> 8e
> > > >>>> yJWIj
> > > >>>> oiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C
> > > >>>> 30
> > > >>>> 00&am
> > > >>>> p;sdata=flQc21b5EVWTy%2Bkmw%2FncIWzdLqUxY5XFislPRs5Ij6o%3D&amp;
> > > >>>> re
> > > >>>> serve
> > > >>>> 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 .
> > >
> > > The order of options and the offset of fields in private struct have 
> > > no effect on ABI. I take these into consideration:
> > > 1. Readability. Related options and fields should be put at the same
> > >    place.
> > > 2. Memory footprint. Reduce struct padding.
> > >
> > 
> > Yes, this is a minor thing within my comment, my comment was mostly regarding the -1 case being unnecessary (since I don't think we need to actually force-force this, just controlling whether this box is written or not under the general rules of where it is defined).
> > 
> > And yes, if the order doesn't matter then grouping makes sense. It's just that for very long "add to the end" was the general principle, so I was mostly utilizing this as a base to request comments from others regarding this.
> > 
> > Jan
> 
> About the order, I agree with what Zhao wrote - I preferred to put it near write_tmcd/write_prft since they are similar.
> I don't mind moving to the end, if that is the decision.
> 
> Regarding the ability to force it, personally, I think that in this case, supporting force doesn't add any complexity to the code, and maybe someone will find it useful at some point. But again, if there is strong objection to this, I will submit a patch without it.
> 
> Thanks,
> 
> Eran
> 

Ping... please let me know if you want me to make these/other changes, would really love to see this getting merged

Thanks

Eran

> > _______________________________________________
> > ffmpeg-devel mailing list
> > ffmpeg-devel@ffmpeg.org
> > https://eur02.safelinks.protection.outlook.com/?url=https%3A%2F%2Fffmp
> > eg.org%2Fmailman%2Flistinfo%2Fffmpeg-devel&amp;data=04%7C01%7C%7C2dc30
> > bb562ec48b1939b08da194d5541%7C0c503748de3f4e2597e26819d53a42b6%7C1%7C1
> > %7C637850117181491627%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQ
> > IjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=YGNFjOd3rL
> > BnswLNfx0YwIOLx%2BXGW6kiL73KfdvHl9I%3D&amp;reserved=0
> > 
> > To unsubscribe, visit link above, or email ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
> >
>
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] movenc: add write_btrt option

[Eran Kornblau]

Another ping...

-----Original Message-----
From: Eran Kornblau 
Sent: Sunday, 17 April 2022 9:47
To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>
Subject: RE: [FFmpeg-devel] movenc: add write_btrt option

> 
> > 
> > On Fri, Apr 8, 2022 at 5:47 AM "zhilizhao(赵志立)" <quinkblack@foxmail.com> wrote:
> > >
> > > > On Apr 8, 2022, at 4:36 AM, Jan Ekström <jeebjp@gmail.com> wrote:
> > > >
> > > > On Thu, Apr 7, 2022 at 11:42 AM Eran Kornblau <eran.kornblau@kaltura.com> wrote:
> > > >>
> > > >>>
> > > >>> -----Original Message-----
> > > >>> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of "zhilizhao(???)"
> > > >>> Sent: Wednesday, 6 April 2022 11:46
> > > >>> To: FFmpeg development discussions and patches 
> > > >>> <ffmpeg-devel@ffmpeg.org>
> > > >>> Subject: Re: [FFmpeg-devel] movenc: add write_btrt option
> > > >>>
> > > >>>> On Apr 3, 2022, at 1:07 PM, Eran Kornblau <eran.kornblau@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
> > > >>>> %2
> > > >>>> Fffmp
> > > >>>> eg.org%2Fpipermail%2Fffmpeg-devel%2F2022-March%2F294623.html&
> > > >>>> am
> > > >>>> p;
> > > >>>> data=
> > > >>>> 04%7C01%7C%7Cb9907958f97048f5645708da17a9f3c8%7C0c503748de3f4
> > > >>>> e2
> > > >>>> 59
> > > >>>> 7e268
> > > >>>> 19d53a42b6%7C1%7C0%7C637848315958196733%7CUnknown%7CTWFpbGZsb
> > > >>>> 3d
> > > >>>> 8e
> > > >>>> yJWIj
> > > >>>> oiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%
> > > >>>> 7C
> > > >>>> 30
> > > >>>> 00&am
> > > >>>> p;sdata=flQc21b5EVWTy%2Bkmw%2FncIWzdLqUxY5XFislPRs5Ij6o%3D&am
> > > >>>> p;
> > > >>>> re
> > > >>>> serve
> > > >>>> 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 .
> > >
> > > The order of options and the offset of fields in private struct 
> > > have no effect on ABI. I take these into consideration:
> > > 1. Readability. Related options and fields should be put at the same
> > >    place.
> > > 2. Memory footprint. Reduce struct padding.
> > >
> > 
> > Yes, this is a minor thing within my comment, my comment was mostly regarding the -1 case being unnecessary (since I don't think we need to actually force-force this, just controlling whether this box is written or not under the general rules of where it is defined).
> > 
> > And yes, if the order doesn't matter then grouping makes sense. It's just that for very long "add to the end" was the general principle, so I was mostly utilizing this as a base to request comments from others regarding this.
> > 
> > Jan
> 
> About the order, I agree with what Zhao wrote - I preferred to put it near write_tmcd/write_prft since they are similar.
> I don't mind moving to the end, if that is the decision.
> 
> Regarding the ability to force it, personally, I think that in this case, supporting force doesn't add any complexity to the code, and maybe someone will find it useful at some point. But again, if there is strong objection to this, I will submit a patch without it.
> 
> Thanks,
> 
> Eran
> 

Ping... please let me know if you want me to make these/other changes, would really love to see this getting merged

Thanks

Eran

> > _______________________________________________
> > ffmpeg-devel mailing list
> > ffmpeg-devel@ffmpeg.org
> > https://eur02.safelinks.protection.outlook.com/?url=https%3A%2F%2Fff
> > mp
> > eg.org%2Fmailman%2Flistinfo%2Fffmpeg-devel&amp;data=04%7C01%7C%7C2dc
> > 30
> > bb562ec48b1939b08da194d5541%7C0c503748de3f4e2597e26819d53a42b6%7C1%7
> > C1 
> > %7C637850117181491627%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLC
> > JQ 
> > IjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=YGNFjOd3
> > rL
> > BnswLNfx0YwIOLx%2BXGW6kiL73KfdvHl9I%3D&amp;reserved=0
> > 
> > To unsubscribe, visit link above, or email ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
> >
>
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] movenc: add write_btrt option

[Eran Kornblau]

[-- Attachment #1: Type: text/plain, Size: 7476 bytes --]

Pinging again... can someone please apply this one? It's a trivial change...

Thanks!

Eran 

-----Original Message-----
From: Eran Kornblau 
Sent: Monday, 25 April 2022 14:26
To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>
Subject: RE: [FFmpeg-devel] movenc: add write_btrt option

Another ping...

-----Original Message-----
From: Eran Kornblau
Sent: Sunday, 17 April 2022 9:47
To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>
Subject: RE: [FFmpeg-devel] movenc: add write_btrt option

> 
> > 
> > On Fri, Apr 8, 2022 at 5:47 AM "zhilizhao(赵志立)" <quinkblack@foxmail.com> wrote:
> > >
> > > > On Apr 8, 2022, at 4:36 AM, Jan Ekström <jeebjp@gmail.com> wrote:
> > > >
> > > > On Thu, Apr 7, 2022 at 11:42 AM Eran Kornblau <eran.kornblau@kaltura.com> wrote:
> > > >>
> > > >>>
> > > >>> -----Original Message-----
> > > >>> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of "zhilizhao(???)"
> > > >>> Sent: Wednesday, 6 April 2022 11:46
> > > >>> To: FFmpeg development discussions and patches 
> > > >>> <ffmpeg-devel@ffmpeg.org>
> > > >>> Subject: Re: [FFmpeg-devel] movenc: add write_btrt option
> > > >>>
> > > >>>> On Apr 3, 2022, at 1:07 PM, Eran Kornblau <eran.kornblau@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
> > > >>>> %2
> > > >>>> Fffmp
> > > >>>> eg.org%2Fpipermail%2Fffmpeg-devel%2F2022-March%2F294623.html&
> > > >>>> am
> > > >>>> p;
> > > >>>> data=
> > > >>>> 04%7C01%7C%7Cb9907958f97048f5645708da17a9f3c8%7C0c503748de3f4
> > > >>>> e2
> > > >>>> 59
> > > >>>> 7e268
> > > >>>> 19d53a42b6%7C1%7C0%7C637848315958196733%7CUnknown%7CTWFpbGZsb
> > > >>>> 3d
> > > >>>> 8e
> > > >>>> yJWIj
> > > >>>> oiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%
> > > >>>> 7C
> > > >>>> 30
> > > >>>> 00&am
> > > >>>> p;sdata=flQc21b5EVWTy%2Bkmw%2FncIWzdLqUxY5XFislPRs5Ij6o%3D&am
> > > >>>> p;
> > > >>>> re
> > > >>>> serve
> > > >>>> 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 .
> > >
> > > The order of options and the offset of fields in private struct 
> > > have no effect on ABI. I take these into consideration:
> > > 1. Readability. Related options and fields should be put at the same
> > >    place.
> > > 2. Memory footprint. Reduce struct padding.
> > >
> > 
> > Yes, this is a minor thing within my comment, my comment was mostly regarding the -1 case being unnecessary (since I don't think we need to actually force-force this, just controlling whether this box is written or not under the general rules of where it is defined).
> > 
> > And yes, if the order doesn't matter then grouping makes sense. It's just that for very long "add to the end" was the general principle, so I was mostly utilizing this as a base to request comments from others regarding this.
> > 
> > Jan
> 
> About the order, I agree with what Zhao wrote - I preferred to put it near write_tmcd/write_prft since they are similar.
> I don't mind moving to the end, if that is the decision.
> 
> Regarding the ability to force it, personally, I think that in this case, supporting force doesn't add any complexity to the code, and maybe someone will find it useful at some point. But again, if there is strong objection to this, I will submit a patch without it.
> 
> Thanks,
> 
> Eran
> 

Ping... please let me know if you want me to make these/other changes, would really love to see this getting merged

Thanks

Eran

> > _______________________________________________
> > ffmpeg-devel mailing list
> > ffmpeg-devel@ffmpeg.org
> > https://eur02.safelinks.protection.outlook.com/?url=https%3A%2F%2Fff
> > mp
> > eg.org%2Fmailman%2Flistinfo%2Fffmpeg-devel&amp;data=04%7C01%7C%7C2dc
> > 30
> > bb562ec48b1939b08da194d5541%7C0c503748de3f4e2597e26819d53a42b6%7C1%7
> > C1
> > %7C637850117181491627%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLC
> > JQ
> > IjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=YGNFjOd3
> > rL
> > BnswLNfx0YwIOLx%2BXGW6kiL73KfdvHl9I%3D&amp;reserved=0
> > 
> > To unsubscribe, visit link above, or email ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
> >
>

[-- Attachment #2: 0001-movenc-add-write_btrt-option.patch --]
[-- Type: application/octet-stream, Size: 4544 bytes --]

[-- Attachment #3: Type: text/plain, Size: 251 bytes --]

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] movenc: add write_btrt option

[Gyan Doshi]

On 2022-05-02 03:03 pm, Eran Kornblau wrote:
> Pinging again... can someone please apply this one? It's a trivial change...

Don't see your patch on Patchwork.

Regards,
Gyan


>
> Thanks!
>
> Eran
>
> -----Original Message-----
> From: Eran Kornblau
> Sent: Monday, 25 April 2022 14:26
> To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>
> Subject: RE: [FFmpeg-devel] movenc: add write_btrt option
>
> Another ping...
>
> -----Original Message-----
> From: Eran Kornblau
> Sent: Sunday, 17 April 2022 9:47
> To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>
> Subject: RE: [FFmpeg-devel] movenc: add write_btrt option
>
>>> On Fri, Apr 8, 2022 at 5:47 AM "zhilizhao(赵志立)" <quinkblack@foxmail.com> wrote:
>>>>> On Apr 8, 2022, at 4:36 AM, Jan Ekström <jeebjp@gmail.com> wrote:
>>>>>
>>>>> On Thu, Apr 7, 2022 at 11:42 AM Eran Kornblau <eran.kornblau@kaltura.com> wrote:
>>>>>>> -----Original Message-----
>>>>>>> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of "zhilizhao(???)"
>>>>>>> Sent: Wednesday, 6 April 2022 11:46
>>>>>>> To: FFmpeg development discussions and patches
>>>>>>> <ffmpeg-devel@ffmpeg.org>
>>>>>>> Subject: Re: [FFmpeg-devel] movenc: add write_btrt option
>>>>>>>
>>>>>>>> On Apr 3, 2022, at 1:07 PM, Eran Kornblau <eran.kornblau@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
>>>>>>>> %2
>>>>>>>> Fffmp
>>>>>>>> eg.org%2Fpipermail%2Fffmpeg-devel%2F2022-March%2F294623.html&
>>>>>>>> am
>>>>>>>> p;
>>>>>>>> data=
>>>>>>>> 04%7C01%7C%7Cb9907958f97048f5645708da17a9f3c8%7C0c503748de3f4
>>>>>>>> e2
>>>>>>>> 59
>>>>>>>> 7e268
>>>>>>>> 19d53a42b6%7C1%7C0%7C637848315958196733%7CUnknown%7CTWFpbGZsb
>>>>>>>> 3d
>>>>>>>> 8e
>>>>>>>> yJWIj
>>>>>>>> oiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%
>>>>>>>> 7C
>>>>>>>> 30
>>>>>>>> 00&am
>>>>>>>> p;sdata=flQc21b5EVWTy%2Bkmw%2FncIWzdLqUxY5XFislPRs5Ij6o%3D&am
>>>>>>>> p;
>>>>>>>> re
>>>>>>>> serve
>>>>>>>> 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 .
>>>> The order of options and the offset of fields in private struct
>>>> have no effect on ABI. I take these into consideration:
>>>> 1. Readability. Related options and fields should be put at the same
>>>>     place.
>>>> 2. Memory footprint. Reduce struct padding.
>>>>
>>> Yes, this is a minor thing within my comment, my comment was mostly regarding the -1 case being unnecessary (since I don't think we need to actually force-force this, just controlling whether this box is written or not under the general rules of where it is defined).
>>>
>>> And yes, if the order doesn't matter then grouping makes sense. It's just that for very long "add to the end" was the general principle, so I was mostly utilizing this as a base to request comments from others regarding this.
>>>
>>> Jan
>> About the order, I agree with what Zhao wrote - I preferred to put it near write_tmcd/write_prft since they are similar.
>> I don't mind moving to the end, if that is the decision.
>>
>> Regarding the ability to force it, personally, I think that in this case, supporting force doesn't add any complexity to the code, and maybe someone will find it useful at some point. But again, if there is strong objection to this, I will submit a patch without it.
>>
>> Thanks,
>>
>> Eran
>>
> Ping... please let me know if you want me to make these/other changes, would really love to see this getting merged
>
> Thanks
>
> Eran
>
>>> _______________________________________________
>>> ffmpeg-devel mailing list
>>> ffmpeg-devel@ffmpeg.org
>>> https://eur02.safelinks.protection.outlook.com/?url=https%3A%2F%2Fff
>>> mp
>>> eg.org%2Fmailman%2Flistinfo%2Fffmpeg-devel&amp;data=04%7C01%7C%7C2dc
>>> 30
>>> bb562ec48b1939b08da194d5541%7C0c503748de3f4e2597e26819d53a42b6%7C1%7
>>> C1
>>> %7C637850117181491627%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLC
>>> JQ
>>> IjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=YGNFjOd3
>>> rL
>>> BnswLNfx0YwIOLx%2BXGW6kiL73KfdvHl9I%3D&amp;reserved=0
>>>
>>> To unsubscribe, visit link above, or email ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>>>
>>
>> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel@ffmpeg.org
>> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>
>> To unsubscribe, visit link above, or email
>> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] movenc: add write_btrt option

["zhilizhao(赵志立)"]

> On Apr 7, 2022, at 4:42 PM, Eran Kornblau <eran.kornblau@kaltura.com> wrote:
> 
>> 
>> 
>> -----Original Message-----
>> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of "zhilizhao(???)"
>> Sent: Wednesday, 6 April 2022 11:46
>> To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>
>> Subject: Re: [FFmpeg-devel] movenc: add write_btrt option
>> 
> Makes sense, thanks for the feedback!
> Updated patch attached
> 

Tested and applied.

> Eran
> <0001-movenc-add-write_btrt-option.patch>_______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".