> On Apr 8, 2022, at 4:36 AM, Jan Ekström <jee...@gmail.com> wrote: > > On Thu, Apr 7, 2022 at 11:42 AM Eran Kornblau <eran.kornb...@kaltura.com> > wrote: >> >>> >>> >>> -----Original Message----- >>> From: ffmpeg-devel <ffmpeg-devel-boun...@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.kornb...@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 > .
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-requ...@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-requ...@ffmpeg.org with subject "unsubscribe".
