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 <[email protected]>
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 <[email protected]>
Subject: RE: [FFmpeg-devel] movenc: add write_btrt option
On Fri, Apr 8, 2022 at 5:47 AM "zhilizhao(赵志立)" <[email protected]> wrote:
On Apr 8, 2022, at 4:36 AM, Jan Ekström <[email protected]> wrote:
On Thu, Apr 7, 2022 at 11:42 AM Eran Kornblau <[email protected]> wrote:
-----Original Message-----
From: ffmpeg-devel <[email protected]> On Behalf Of
"zhilizhao(???)"
Sent: Wednesday, 6 April 2022 11:46
To: FFmpeg development discussions and patches
<[email protected]>
Subject: Re: [FFmpeg-devel] movenc: add write_btrt option
On Apr 3, 2022, at 1:07 PM, Eran Kornblau <[email protected]> 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
[email protected]
https://eur02.safelinks.protection.outlook.com/?url=https%3A%2F%2Fff
mp
eg.org%2Fmailman%2Flistinfo%2Fffmpeg-devel&data=04%7C01%7C%7C2dc
30
bb562ec48b1939b08da194d5541%7C0c503748de3f4e2597e26819d53a42b6%7C1%7
C1
%7C637850117181491627%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLC
JQ
IjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=YGNFjOd3
rL
BnswLNfx0YwIOLx%2BXGW6kiL73KfdvHl9I%3D&reserved=0
To unsubscribe, visit link above, or email [email protected] with subject
"unsubscribe".
_______________________________________________
ffmpeg-devel mailing list
[email protected]
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
[email protected] with subject "unsubscribe".
_______________________________________________
ffmpeg-devel mailing list
[email protected]
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
[email protected] with subject "unsubscribe".
