michaelni changed the topic of #ffmpeg-devel to: Welcome to the FFmpeg development channel | Questions about using FFmpeg or developing with libav* libs should be asked in #ffmpeg | This channel is publicly logged | FFmpeg 7.1 has been released! | Please read ffmpeg.org/developer.html#Code-of-conduct
rvalue- has joined #ffmpeg-devel
rvalue has quit [Ping timeout: 272 seconds]
rvalue- is now known as rvalue
Kei_N has joined #ffmpeg-devel
Kei_N_ has quit [Ping timeout: 260 seconds]
iive has quit [Quit: They came for me...]
_whitelogger_ has quit [Remote host closed the connection]
Traneptora has quit [Quit: Quit]
Traneptora has joined #ffmpeg-devel
_whitelogger_ has joined #ffmpeg-devel
Traneptora has quit [Quit: Quit]
tufei_ has quit [Quit: Leaving]
IndecisiveTurtle has quit [Ping timeout: 260 seconds]
tufei has joined #ffmpeg-devel
_whitelogger_ has quit [Remote host closed the connection]
tufei has quit [Ping timeout: 260 seconds]
_whitelogger_ has joined #ffmpeg-devel
thilo has quit [Ping timeout: 245 seconds]
thilo has joined #ffmpeg-devel
arch1t3cht4 has joined #ffmpeg-devel
arch1t3cht has quit [Ping timeout: 272 seconds]
arch1t3cht4 is now known as arch1t3cht
cone-056 has joined #ffmpeg-devel
<cone-056>
ffmpeg Marth64 master:7332b1700e3b: avformat/sapdec: check return value of avcodec_parameters_copy()
_whitelogger_ has quit [Remote host closed the connection]
_whitelogger_ has joined #ffmpeg-devel
<aaabbb>
as part of https://fftrac-bg.ffmpeg.org/ticket/8343 i'm reading through h.261 specs about whether the freeze-picture release bit must be set for intraframes because if so, then the h261dec can be made to set AVFrame::key_frame whenever that bit is set (h261enc sets that bit when doing keyframes). basically i am trying to find out if michaelni's objections in
_whitelogger_ has quit [Remote host closed the connection]
<aaabbb>
it looks to me like there's no explicit way to signal an intraframe in h.261 although there are some signals that guarantee that the next frame is one. because of how simple h261 is, could it be enough to mark any frame with 100% intra-blocks as a keyframe, and have h261dec emit AVFrame:key_frame when the whole frame is all intra-blocks?
_whitelogger_ has joined #ffmpeg-devel
<compn>
let h261 dieeeeeeeeeeeee /s
<compn>
i think mencoder uses -ovc lavc and then -lavcopts vcodec=h261 so that would be ffmpeg's h261 encoder
<aaabbb>
lots of illegal acs and macroblock errors
^Neo has quit [Ping timeout: 252 seconds]
Martchus has joined #ffmpeg-devel
Martchus_ has quit [Ping timeout: 252 seconds]
System_Error has quit [Remote host closed the connection]
System_Error has joined #ffmpeg-devel
\\Mr_C\\ has quit [Remote host closed the connection]
_whitelogger_ has quit [Remote host closed the connection]
_whitelogger_ has joined #ffmpeg-devel
_whitelogger_ has quit [Remote host closed the connection]
_whitelogger_ has joined #ffmpeg-devel
<fflogger>
[newticket] kxxt: Ticket #11302 ([avcodec] RISCV: Properly set relocate parameter of const macro in libavcodec/riscv/h264dsp_rvv.S) created https://trac.ffmpeg.org/ticket/11302
cone-056 has quit [Quit: transmission timeout]
<fflogger>
[editedticket] kxxt: Ticket #11302 ([avcodec] RISCV: Properly set relocate parameter of const macro in libavcodec/riscv/h264dsp_rvv.S) updated https://trac.ffmpeg.org/ticket/11302#comment:2
_whitelogger_ has quit [Remote host closed the connection]
_whitelogger_ has joined #ffmpeg-devel
System_Error has quit [Remote host closed the connection]
System_Error has joined #ffmpeg-devel
System_Error has quit [Remote host closed the connection]
System_Error has joined #ffmpeg-devel
HarshK23 has joined #ffmpeg-devel
_whitelogger_ has quit [Remote host closed the connection]
System_Error has quit [Ping timeout: 260 seconds]
cone-813 has joined #ffmpeg-devel
<cone-813>
ffmpeg Peter Ross master:6ccd16dda2e1: configure: add rv60 decoder golomb dependency
<cone-813>
ffmpeg Peter Ross master:f7e7d63cbcf3: avcodec/rv60: simplify fill_mv_skip_cand
<cone-813>
ffmpeg Peter Ross master:566b737a7a7a: avcodec/rv60: loosen fill_mv_skip_cand top right and bottom left criteria
<cone-813>
ffmpeg Peter Ross master:7b3bc4a45ba0: avcodec/rv60: consistent indentation
_whitelogger_ has quit [Remote host closed the connection]
_whitelogger_ has joined #ffmpeg-devel
Traneptora has joined #ffmpeg-devel
mkver has joined #ffmpeg-devel
j45_ has joined #ffmpeg-devel
j45 has quit [Ping timeout: 248 seconds]
j45_ is now known as j45
j45 has quit [Changing host]
j45 has joined #ffmpeg-devel
<fflogger>
[newticket] ff_tux: Ticket #11303 ([undetermined] AAC files are always reported as constant bitrate) created https://trac.ffmpeg.org/ticket/11303
cone-813 has quit [Quit: transmission timeout]
<fflogger>
[editedticket] michael: Ticket #5548 ([avcodec] FFV1 encoder creates invalid stream with -level 3 if width or height is between 1 and 3) updated https://trac.ffmpeg.org/ticket/5548#comment:3
<fflogger>
[editedticket] michael: Ticket #11128 ([avcodec] FFV1 encodes files that fail strict 3rd-party MediaConch validation when using 2-pass encoding) updated https://trac.ffmpeg.org/ticket/11128#comment:4
^Neo has joined #ffmpeg-devel
^Neo has quit [Changing host]
^Neo has joined #ffmpeg-devel
<BBB>
elenril: I had the same reaction to that x265 post, btw. I still don't think we should delete it (or flag it experimental) but I agree with the "WTF" sentiment
<JEEB>
now to recall where the heck picture crop was in HEVC parameter setds
<elenril>
BBB: a codec of which you cannot have two instances should be _at least_ experimental
<elenril>
and with a fat blinking runtime warning
jamrial has joined #ffmpeg-devel
cone-569 has joined #ffmpeg-devel
<cone-569>
ffmpeg Michael Niedermayer master:967e5e8f6f6b: avcodec/ffv1: Support slice coding mode 1 with golomb rice
<cone-569>
ffmpeg Michael Niedermayer master:a5c0ed2122a9: avcodec/ffv1: Support >8bit rice golomb
<cone-569>
ffmpeg Michael Niedermayer master:0b8c9cf5b490: avformat/movenc: Fix ffv1 support
<JEEB>
llyyr: literally this made it output something that was flagged 1920x1080 -init_hw_device vaapi=foo -filter_hw_device foo -i INPUT -vf hwupload,format=vaapi
<jamrial>
is that not already supported? for amd i mean
<JEEB>
llyyr: then there's a separate boog where the result of that yuv420p into the encoder is lulzy pink, but it is 1920x1080 pink :D
<llyyr>
:shrug:
<JEEB>
(if you then add ,scale_vaapi=format=nv12 it's perfect)
<JEEB>
thus for whatever goddamn reason it's dependent on the type of VAAPI surface the encoder is getting
lemourin has joined #ffmpeg-devel
<JEEB>
I'll see if I can see anything interesting in that AVFrame
<JEEB>
ah wait no, the vaapi scale one is 1088
<JEEB>
OK, ignore me - how the heck did I believe I saw 1080
<cone-089>
ffmpeg Marton Balint master:e9af7b7b5d38: avutil/opt: do no rely on av_d2q returning higher num/den than allowed when parsing rational opts
<cone-089>
ffmpeg Marton Balint master:25efe34e6f13: avutil/parseutils: do no rely on av_d2q returning higher num/den than allowed in av_parse_video_rate
<cone-089>
ffmpeg Marton Balint master:d884606ce0f4: avutil/rational: never return greater num/den than the maximum allowed in av_d2q
Everything has joined #ffmpeg-devel
Krowl has quit [Read error: Connection reset by peer]
<BtbN>
Like I said, at least one person or group tried to re-invent ffmpeg in rust. And found it to be too slow to be useable, cause of all the checks.
<names_are_hard>
Currently I don't have the root cause diagnosed, I'm in the middle of a fuzzing run. I don't want to file an incomplete bug and you don't want to read it, so it'll happen later
<BtbN>
FFmpeg is as fast as it is because of optimized code like this. And hand-written assembly. That also doesn't do null checks and assumed valid input.
<names_are_hard>
Yeah, sure, I get it
<BtbN>
If you have a sample that reproducably crashes, you got one whole bug right there.
<names_are_hard>
You want to do hyper optimised stuff and don't care about security, it's your choice
<BBB>
it is ok to file a sample that crashes
<BtbN>
Stuff _is_ checked
<BBB>
we get a fair number of them and we try to fix them asap
<BtbN>
it just needs to be done in the right place
<BBB>
we really care about security
<BtbN>
not in the middle of what is effectively a dsp
<BBB>
exactly
<BBB>
there is design to this stuff. we're not a bunch of newbies here :)
<names_are_hard>
Well, this sample relates to the actual feature change I want to make, and I haven't tested that the code is reachable without my changes (logically, it should be, but weirder things have happened)
<BtbN>
If the sample crashes vanilla ffmpeg, it's a valid bug all on its own
<BBB>
ah, so this is not on git/master?
<BBB>
right, that complicates things ... a bit :)
<names_are_hard>
It doesn't, but I would guess lossless jpeg shopuld also be able to hit it. Currently unproven, I will investigate, since you (sensibly!) won't take my feature with outstanding crash bugs
<names_are_hard>
That's why I asked about where to fix it
<names_are_hard>
And was told I was an idiot for even wanting to fix it at the point of use. Good old reliable internet
<BBB>
there's a few practical concerns about doing it in the idct
<names_are_hard>
Yeah, want to make the change and profile it?
<names_are_hard>
Because I bet it isn't noticeable
<BBB>
no, we don't want you to profile it
<BBB>
it's bad design
<names_are_hard>
Checking for null pointer before usage is good design
<BBB>
no
<names_are_hard>
There are multiple ways to do that
<BBB>
second, it means the check has to be reproduced in *every* simd implementation
<BtbN>
We won't be adding one random null check in dsp code to work around a bug
<BBB>
that is really dumb design
<BtbN>
You could argue adding A TON of null checks, but not just one random one
<names_are_hard>
*if* you can guarantee that all paths into the func in question already checked for nulls, then you *are* checking for null before usage
<BBB>
for (int y = 0; y < h; y++) { for (int x = 0; x < w; x++) { idct( -- do NULL check here? --); } }
<BBB>
or
<BBB>
do null chck here; for (int y = 0; y < h; y++) { for (int x = 0; x < w; x++) { idct(); } }
<BBB>
which is better design
<BtbN>
static null checks would be interesting, but I don't think C can offer that
<BBB>
names_are_hard: would you like your patch to be accepted in this project?
<BBB>
names_are_hard: if the answer is yes, then my suggestion is to not add a null check in a simd function
<names_are_hard>
I have never suggested I will be adding that check
<BBB>
good :)
<BBB>
second, we'd want a way to reproduce anyway
<names_are_hard>
I am going to do the absolute minimum of effort to meet what I'm told is required
<names_are_hard>
I'm trying to make a 6 line change to improve support for a existing supported file format, and it's taken me over a month so far
<names_are_hard>
This isn't an easy project to work with
<names_are_hard>
(which is what I expected: it's a large, complex project)
<BBB>
I don't know how feasible this suggestion is, but it's usually easier to get help if you show code or something like that
<BtbN>
You can post patches as RFC, yeah
<BtbN>
specially if you aren't sure about stuff someone who is familiar with the code could probably answer near instantly
<names_are_hard>
I posted patches, I was told I needed to fuzz inputs to test code before they could be accepted
<names_are_hard>
And it took several weeks to get replies on the list
<BtbN>
We don't have that requirement. Fuzzing every patch would be insane
<BtbN>
well, it seems like he was right, and you found one?
<BtbN>
So post it as an answer, and ask for advice
<BtbN>
With the sample ideally, if it's not too large/legally problematic
<names_are_hard>
I don't yet know if this is sample demonstrates a bug with my changes, or with earlier LJ92 code, or with something else
<BtbN>
Well, there's people who might. But if you don't share it and what you found, you won't find out
<names_are_hard>
I will find out, because I will diagnose it
<names_are_hard>
I'm an experienced C dev with a lot of experience with fuzzers
<BtbN>
It's definitely a nieche thing to work on. So it's normal that there's no huge response
<names_are_hard>
I'd rather understand the sample before posting it (maybe it's a mistake, or maybe it's a serious vuln; in either case you don't want me posting it to a public list)
<BtbN>
If it doesn't crash with vanilla ffmpeg, I see no reason not to share it publicly
<names_are_hard>
There's a good reason it might not
<names_are_hard>
I'm forcing the header bytes in the file to be MLV format, and vanilla doesn't parse LJ92 compressed MLV (that's what my changes are)
<BtbN>
Yeah, so that's absolutely safe to share then
<names_are_hard>
No, it isn't
<names_are_hard>
The underlying bug may be in LJ92 parsing, which other files can hit
<names_are_hard>
But vanilla won't get to those paths with MLV, without my changes
<names_are_hard>
The ideal testcase would be LJ92 jpeg
<names_are_hard>
Assuming it's not a bug with my changes
<BtbN>
Send the sample privately then and refer to it.
<BtbN>
Bumping your head against a wall for a long time seems just needlessly frustrating
<names_are_hard>
I'm not doing that
<names_are_hard>
I was asking where the right place to fix the null pointer deref was. Which you have answered, even though I find the answer surprising
<BtbN>
You fix it as early as possible, yeah
<BtbN>
not in the middle of ultra-hot code
<BtbN>
That stuff is often benchmarked in "cpu cycle count"
<names_are_hard>
Or, crazy idea: fix it in both places if the profiler says you can afford it
<BtbN>
so of course extra checks will be moved out of it
<BtbN>
Again, by that logic, we'd need to add literal thousands of tens of thousands of checks everywhere. Which then would hurt performance significancly.
<names_are_hard>
Would it though? Have you measured this?
<names_are_hard>
Anyway, really, I don't care about that
<BtbN>
Yes, those functions are extensively benchmarked all the time
<names_are_hard>
With the checks?
<BtbN>
By counting single CPU cycles
<BtbN>
Also, most of those functions have mulitple implementations. The C one is just the basic fallback
<BtbN>
there's potentially dozens of handwritten assembly variants of them
<names_are_hard>
It's fine, I value correct core more than fast code, you don't - but it's your code so do whatever
<BtbN>
You simply don't realize the magnitudes here
<BtbN>
this stuff is called A LOT
<names_are_hard>
Sure, call me an idiot again, that'll help
System_Error has quit [Ping timeout: 260 seconds]
<BtbN>
No idea where you are reading anyone calling you an idiot here
<names_are_hard>
I'm saying *there is a tradeoff*
<BtbN>
And I'm saying you are hugely underestimating the magnitude of that tradeoff
<names_are_hard>
No
<names_are_hard>
I'm valuing correctness more than performance, compared to you
<names_are_hard>
Which is fine! It's allowed to choose anywhere on the line!
<BtbN>
Checking the same pointer for null trillions of times does nothing for correctness
<names_are_hard>
I was never asking to do that
<BtbN>
You are though, by suggesting to add null checks in the deep middle of dsp code
<names_are_hard>
Prove it's never null and the job is done, I don't mind how
<BtbN>
You can't prove that nobody will ever write a bug that calls it with an invalid value
<BtbN>
The functions are written under the assumption that the caller took all neccesary precautions
<names_are_hard>
Which is a trade off for performance at the cost of correctness
<names_are_hard>
Because people make mistakes
<BtbN>
If you want to write a useable decoder, those are neccessities
<names_are_hard>
That's your claim; I don't believe it
<BtbN>
It easily means the difference between being comfortably real time capable, or crawling along with a handful of FPS
<names_are_hard>
I don't believe this claim
<names_are_hard>
It also doesn't matter whether I do or not
<BtbN>
Those function can be easily called literally tens of thousands of times or more per frame.
<BtbN>
So while one null check might not yet make a huge difference (though you can probably already measure or even see it), adding all of them to make it follow your idea, would grind it to a halt
<names_are_hard>
I think you're wrong
<BtbN>
And you base that on what?
<names_are_hard>
Years of experience
<names_are_hard>
I'm not saying I'm confident, I'm just expressing my opinion
<BtbN>
In dealing with media processing?
<names_are_hard>
Here's how to prove me wrong: add the null checks and profile it
<names_are_hard>
I don't care enough to bother, it's not my code
<BtbN>
I'm not gonna waste potentially multiple days of my time to prove a point.
<names_are_hard>
Sure, that's reasonable
<BtbN>
I'd also have to learn assembly first
<names_are_hard>
This is a C function
<BtbN>
You are still not getting the point
<BtbN>
It's not about adding that one random null check
<names_are_hard>
I never said it was
<BtbN>
You just said "this is a C function".
<names_are_hard>
It seems to me you're arguing against a point I never made?
<BtbN>
I'm saying that doing null and range checks in dsp code is slow. Since it's called a hell of a lot of times per frame.
<names_are_hard>
And I said this was a C function because you said you'd have to learn asm; you would not, for profiling this one specific change. If you want to profile more, sure, maybe you have to learn more. That's up to you
<BtbN>
You still don't get it. It's not about this one random null check.
<BtbN>
It's about the whole principle of performing checks in this kind of code
<names_are_hard>
I have never, and am not at this time, saying it is about a single null check
<BtbN>
Are you trolling? You just contradicted yourself within two messages oO
<names_are_hard>
Sorry, I don't understand what you mean
<another|>
names_are_hard: Then what are you saying? It's about ALL of the check that would need to be addede?
<names_are_hard>
No, BtbN is saying it's about all checks. But checks I'm not talking about
<another|>
Then what _are_ you talking about?
<BradleyS>
checking the incorrect assumptions in this argument is about as expensive as checking null in dsp code
<BtbN>
You just said "It's just about this one check" and "I have never said it's about a single null check" back to back. So what is it?
<another|>
BradleyS: lul
<names_are_hard>
Where did I say "It's just about this one check"?
<BtbN>
The literal message before
<names_are_hard>
What's the timestamp?
<BtbN>
The literal same minute
<another|>
BtbN: I don't see it either
<BtbN>
"And I said this was a C function because you said you'd have to learn asm; you would not, for profiling this one specific change."
<names_are_hard>
Thanks. I'm not sure how you're interpreting that, but quoting me saying something else didn't help me understand
<BBB>
why are we still discussing this :-/
<names_are_hard>
Hell if I know
<BtbN>
I've told you repeatedly that it's not about one singular null check
<BBB>
it's ok to share the sample. really, it is
<names_are_hard>
Yeah, I will, that's not changed
<BBB>
point to the github repo (or whatever) if you can't repro on git/master
<BtbN>
One single null check is going to be hard to measure. But adding one random null check to work around an issue elsewhere is, again, just silly and not going to happen.
<BBB>
and if there is a bug in upstream lj92 that is not accessible through this format (yet) but is-still-a-bug, then it's ok
<another|>
names_are_hard: the change in C would have to be replicated across several asm implementations
<names_are_hard>
Do you have existing lossless jpeg samples? That would save me some time
<names_are_hard>
I've got FATE working locally so I did the sample sync step
<BBB>
fate-suite//jpegls/* has a bunch of files in it
<BBB>
they are lossless jpeg?
<names_are_hard>
Should be, thanks
<BBB>
make fate-suite-rsync in your git repo to download it
<BBB>
make sure you have the fate samples path set
<BBB>
(configure --samples=/path/to/samples)
<names_are_hard>
It's late here, I'll kill this fuzz run, roll back to vanilla and try that on the jpegs
<BBB>
sounds good, tnx
System_Error has joined #ffmpeg-devel
mkver has quit [Ping timeout: 276 seconds]
<elenril>
>23:37 <names_are_hard> Why don't we want to check at point of use? That's generally the saner design
<elenril>
I disagree about that
<elenril>
and not even because of any perofrmance concerns
<names_are_hard>
I could easily be biased, my background involves writing fairly paranoid code
<BtbN>
specially in a function that has no way to signal error
<BtbN>
it'd just create even more invalid state
<names_are_hard>
I suppose the middle ground would be something like ASSERT(some_ptr != NULL), you don't pay any cost on non-debug builds
<elenril>
as BtbN said, if you get there with a NULL buffer, something went wrong much, much earlier
<elenril>
the right solution is to find where and fix it there
<names_are_hard>
Sure, I agree with that - but don't you want to find that?
<elenril>
of course
<BtbN>
And how would hiding the crash help with that?
<BtbN>
You don't even get a backtrace then, just potentially random runtime behaviour
<elenril>
my point is just that sprinkling random checks in random places does not address the issues, it just hides it
<names_are_hard>
Why are you assuming I want to hide a crash?
<elenril>
because that's what you said above
<names_are_hard>
Did I?
<elenril>
at least that's how multiple people here read it
<names_are_hard>
You can make ASSERT do whatever you want. I was arguing for checking null pointers before usage; what you do if the check fails I didn't give an opinion on
MajorBiscuit has joined #ffmpeg-devel
<names_are_hard>
And of course; writing through a null pointer may not crash. Which is my real point. And what your code does currently
<BtbN>
They are checked before usage. Just not as granular as you are asking for here.
<names_are_hard>
They're *sometimes* checked before usage, but clearly not always