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.1 has been released! | Please read ffmpeg.org/developer.html#Code-of-conduct
<beastd>
michaelni,kierank,BtbN,JEEB: The commit message rules are partially written down in developer.texi where I looked at them a few days ago. I agree having it checked in CI is good, I partially disagree with the implementation. Should also probably not be a hard merge blocker (which it wasn't as AFAIU)
<JEEB>
yea it was not, wbs was able to merge with his capital letter starting initial description post-prefix just fine
<beastd>
think we should refine and improve what was there. also if it wasn't easy to do it locally, it should be implemented in a way that it is accessible to developers that care locally
<JEEB>
and yes, I am fine with people having issues with the exact implementation, just that someone commenting that the linted ruleset is sudden and/or arbitrary is a case of "huh, what?". it's as if there is some other issue that they are not happy about and decided to lash out regarding whatever is the latest possible issue
<JEEB>
I would assume the people who added that commit message linter did it in good faith and looking at the general practice (most commits, possibly the developer docs that were linked), and it was then reviewed by some people when it was added. I had no play in it for the record.
<JEEB>
beastd: seems to be a python script so I would expect that it could be similarly run locally
<JEEB>
it does base on various GITHUB_* environment variables, as it's trying to limit things to your PR
<JEEB>
(to get the commits from your PR)
<JEEB>
so that part might not be as easily usable, but if you only want it to check the current commit that should be possible to achieve
<JEEB>
oh, it has local mode :) if GITHUB_EVENT_NAME is not set to pull_request, it requires you to provide an argument for commit range
GewoonLeon has joined #ffmpeg-devel
<JEEB>
which then calls `git log -z --pretty=format=%H%n%B COMMIT_RANGE` to get the stuff
<wbs>
JEEB: no, for that commit I didn't bother and changed it to a lower case letter
<wbs>
but I also disagree with that rule. codifying "common practice" may be fine, but something that is done by 59% percent of commits is not "the vast majority"
<JEEB>
oh
<wbs>
I don't mind other linting rules, but that one I disagree with
<wbs>
discuss it and make it a formal policy, sure, I'll follow it
<wbs>
but codifying it as "the vast majority", based on 59%, then no
<JEEB>
:)
<JEEB>
I do wonder where various people then learned that practice
<JEEB>
because while yes, the examples in developer docs exactly do that, you could say "those are just examples"
<wbs>
IMO, common English sense - sure it's not a full sentence (and hence no full stop), but it's a title of a thing, and titles start with a capital letter
<JEEB>
(I actually don't recall myself, only recall the "it's not a sentence, thus no capital letter and no dot at end" explanation from *somewhere*
<beastd>
Start with capital letter and don't end with a full stop is sure a practice used in git.git
<beastd>
And that is where usually I got my primary inspiration about how to do Git from
CoreX has quit [Server closed connection]
<JEEB>
yea noticed kernel possibly has that as the practice looking at one "how-to contribute to kernel" has a screenshot of such a message
<beastd>
JEEB: I think the lint commits script could work well locally. Also doesn't have many deps. would just rename it (e.g. like in its self documentation) and put it under tools/
<JEEB>
beastd: yea, just argv[1] for example HEAD~1..HEAD should do OK
<TheVibeCoder>
ffmpeg is very buggy
Teukka has quit [Read error: Connection reset by peer]
<fjlogger>
[FFmpeg/FFmpeg] Pull request #20154 opened: WIP: avutil/pixdesc: reduce the size of symbols in pixdesc to optimize code size. (https://code.ffmpeg.org/FFmpeg/FFmpeg/pulls/20154) by Renjianguang-mi
<beastd>
Think it's good that all PRs get send to the ML, but I would say another mail could be useful. A mail that is sent when the PR is ready and intended to be merged soon.
<beastd>
Now that is not a real concept in forges AFAICT, so we would need to implement it in some way. Maybe just with labels on that PR that get added and in turn cause the notification to get send to the ML.
<JEEB>
there should possibly be a notification for PR being accepted
<kasper93>
it's about consistency, if you feel like most commits are wrong, so be it
<BtbN>
JEEB, beastd: The script was specifically designed to be able to run locally
<kasper93>
but leaving it random, is not fixing the problem, it's only turning blind eye on it
<BtbN>
if the GITHUB env vars are not available it just needs you to pass the commit range as parameter
<wbs>
kasper93: I don't have a very strong opinion on it. I do like consistency. but if the argument was "the vast majority" for adding undiscussed policy, while the "vast majority" was 57% then I disagree with that assessment
<kasper93>
anyway, it's one way or anothers, cannot have it both ways.
<BtbN>
I very much think commit messages are better starting without a capital letter, unless it's something like an acronym, then of course it should be capitalized like usual
<BtbN>
Hence making the checks mandatory is a bad idea, since there will always be reasonable exceptions
<beastd>
BtbN: good to hear. we saw that. it was just not so obvious because of placement in .forgejo and slightly wrong doc (also would be clearer to not detect this inside get_commits with by accessing env vars and args from there)
<BtbN>
How else would you detect that?
<BtbN>
That's exactly how you detect running in an actions runner
<wbs>
kasper93: BtbN said "vast majority" as argument for enabling the check without discussion
<wbs>
I don't mind bending to this policy, if we agree upon it
<BtbN>
It is the vast majority if you "only" look at the last 5 or 10 years of commits
<wbs>
no?
<BtbN>
The huge majority of deviations are in the early days of the project
<wbs>
did you see kasper93's numbers?
<beastd>
BtbN: I mean you do it inside get_commits func. I would do it in main or maybe even do it in a script calling it for CI (external to the generic script)
<wbs>
it is literally 57% of lowercase in the last 5 years or so
<wbs>
it's slightly more lowercase if you include the whole history
<BtbN>
beastd: so you want to wrap the script in another script, just to get the commit range? That seems a bit unneccesary.
<BtbN>
That's not what was in the stats posted tonight.
<beastd>
BtbN: maybe. just trying to suggest solutions. could also be outside of script and in the adhoc script parts in the yaml maybe
<wbs>
since first commit, 64% lowercase
<BtbN>
Oh, I misread that. I thought it said 98% match the pattern.
<BtbN>
beastd: I don't see an issue with the get_commits function to, well, get the commits
<wbs>
do you now understand why I feel mildly annoyed that this goes through with "the vast majority"?
<BtbN>
It is still the majority
<wbs>
yes
<wbs>
but not "vast"
<BtbN>
And also imo good practice
<BtbN>
But I don't care about it enough to fight over it
<wbs>
I disagree about that, there's lots of projects that prescribe upper case
<wbs>
I also totally agree about that bit
<BtbN>
for all I care we can just keep commit message wild west, but we then gotta have to explain it over and over to new contributors, as already demonostrated today
<wbs>
that it's a waste of time discussing
<kasper93>
And there is a lot of projects to prescribe lower case, ffmpeg happens to be one of them, in the docs itself
<BtbN>
Cause clearly writing it in the guidelines is not enough for people to read it before submitting
<wbs>
kasper93: not explicitly prescribed, only implicitly through one example, no?
<beastd>
BtbN,wbs: I too prefer: [<area>:] Do this or that <- starting with a capital and ending without a dot
<BtbN>
I think the capital looks very out of place and looks outright wrong
<BtbN>
it's not a sentence
<kasper93>
wbs: well there is no commit message police. But it's shown as good commit message, so understanably, every contributor that reads it will understand that this is how commit should look, no?
<beastd>
This is not a problem it starts after a : or at the beginning. also the whole thing is like a heading
<wbs>
kasper93: well apparently 40% of recent contributions haven't picked that up
<wbs>
including long time major contributors
<TheVibeCoder>
hacks and style issues plague ffmpeg codebase
<beastd>
actually i think we should not care if it starts with lower case or upper case. just not that important after all i think. so we could just leave it open
<wbs>
kasper93: in any case, as we see there are strong opinions in both directions - so I wouldn't say it can be settled by just looking at numbers. get it agreed upon and documented as _explicit_ projectwide policy and I'll oblige without whining
<kasper93>
if you loko at michael, it's 48.62% vs 50.86%, he switched at some point ;)
<Sean_McG>
I smell a bikeshed
<kasper93>
wbs: sure, no need to change anything
<wbs>
other than that, I don't mind what other things the linter script checked (although I didn't dig it through in detail), it's just this one detail which turned out to be controversial
<kasper93>
I only started looking at lintng stuff, because first thing after switching to forgejo, was breaking v.d.o repository, because not cleaned patch was merged
<BtbN>
I'm also still really not sure if that stuff should be in fate. Why should fate runners test already set in stone code style and commit messages?
<Sean_McG>
^
<Sean_McG>
especially given how many nodes we have, that would be overkill
<wbs>
yeah I'm not entirely sure about that either, but michael at least wants some way to fully test everything locally before pushing
<BtbN>
You can though
<TheVibeCoder>
mini is bussy with non-ffmpeg backlog
<Sean_McG>
do commit hooks run locally?
<BtbN>
you can, it's pre-commit
<beastd>
BtbN: Think it should NOT be in fate. Just easily discoverable und usable locally
<BtbN>
if you install pre-commit and run "pre-commit install" in the ffmpeg repo, it will automatically run... pre-commit
<BtbN>
And the commit linting script also was plain python without dependencies designed to run locally or on CI
<BtbN>
the main challenge with automatically running it locally is finding the right range of commits to check
<JEEB>
yea
<JEEB>
you could default to HEAD~1..HEAD, which only checks current commit
<BtbN>
I tried a lot of methods, and they all failed
<beastd>
yes, but that is fine. was just nitpicking about "Usage: ./lint_commits.py <commit-range>" and location of the script
<BtbN>
Like, my initial idea was "if on master or release/* just check HEAD commit", and if not, find merge base with master and check until then
<BtbN>
but then I immediately proved myself wrong, cause I almost never update my master branch, so it checked hundreds of commits
<JEEB>
yea, easier to just default to HEAD
<JEEB>
you're most often interested in the commit you just made, after all
<BtbN>
I think you're most often interested in the commitS you just made
<kasper93>
I do @{upstream}.. to get my commits starting at upstream merge base
<BtbN>
problem is finding them :D
<JEEB>
sure
<JEEB>
exactly
<BtbN>
yes, but the script can't assume everyone has an upstream branch and it's up to date
<BtbN>
*remote
<JEEB>
thus HEAD~1..HEAD should be a Good Enough default if someone wants such
<kasper93>
yes, that's why I also think it's not the job for `make fate`
<beastd>
just checking the current commit and leaving it up to the user to specify it better is fine imho
<BtbN>
One idea I'd have is "check HEAD commits and all commits prior to it from the same author or until merge base with master"
<kasper93>
although `pre-commit` could be in fate, especially line endings checks
<BtbN>
We already have fate-source, or something like that?
<BtbN>
Which checks license headers or something
<kasper93>
pre-commit itsef is just a wrapper on things, it's covinient, but alternatives are valid too
<kasper93>
we just need to implement each check and same for calling codespell
<BtbN>
Given the weird attitude of the pre-commit maintainers, I'm all for NIH'in it
<BtbN>
we're only doing pretty basic checks anyway
<kasper93>
source-check.sh is the most random script I've seen
<kasper93>
let's check licence and while at it av_clip() usage too
<beastd>
BtbN: about the range: I think that is over thinking. just leave it to the user to specify it. we could give examples for commen cases like `$(git merge-base origin/master HEAD)..`
<BtbN>
beastd: well, the idea is that it runs automatically with "make fate"
<BtbN>
so there is no option of passing in the commit range there
<JEEB>
yea then it would just be HEAD~1..HEAD
<beastd>
ah you are thinking of that
<beastd>
i think it's wrong to run it on make fate. should be separate
<kasper93>
I'd like also to point out, that what we are doing here is fighting agains forgejo.
<kasper93>
Those issues we try to solve now, are exact issues solved by using code forge.
<beastd>
could also run additional things like patcheck that do not fit into the fate target
<kasper93>
what is the merge base?
<kasper93>
what files to lint?
<kasper93>
where is code merge?
<kasper93>
and so on
<beastd>
kasper93: yes and no. this should be checked in forgejo, but developers should have the ability to check it locally too if they desire.
<beastd>
it should actually become a bit of a habit for maintainers for non-trivial stuff. nothing more annoying then to see you have messed up commit messages after (force) pushing to the forge and seeing that quite a bit later when the job went thru the queue
<kasper93>
yes, I agree. That's why I didn't want this lint script to be in github-script
<BtbN>
kasper93: I think being able to locally verify Forgejo will be happy very much has merit, and saves you one cycle of iteration
<JEEB>
yea, locally current commit (or running with a range with arguments), and PR commit set on forgejo
<kasper93>
we can very quickly add `make fate-codespell fate-pre-commit` and remove pre-commit later
<BtbN>
It's IMO pretty impressive how much activity is already happening on code.ffmpeg.org, without a full on official announcement. And so far I have seen none of the proclaimed "Newbie code" or whatever Nicolas is afraid of
<Lynne>
BtbN: what if we just cut the ML link, ban him on forgejo and just let him drown out the ML with enough garbage until its bannable
<Sean_McG>
indeed.
<BtbN>
Lynne: he has a forgejo account outside of the test dummy?
<Lynne>
yeah, Cigaes
<BtbN>
Never seen him write anything with that before
<BtbN>
though I also did not read all 150 notifications from tonight
<Lynne>
its his nick he uses when he has to
<Lynne>
he used to use it here on IRC
<Lynne>
...where he complained about how decisions must be done on the ML
<BtbN>
Banning him in Forgejo seems unnecessary unless he gets abusive or something there
<BtbN>
I feel like nobody is taking him for full anymore, he's just shouting into the void
<beastd>
BtbN: Our forgejo and workflow still has rough edges, but the participation and progress is amazing! Really significantly more than I initially anticipated for the (pre-)beginning. Thanks for working on this relentlessly :)
<Sean_McG>
110% thanks BtbN
<sfan5>
I'm really not sure what is it with the ffmpeg project and not removing people who are clearly detrimental to the developer community. Not really a good look for outsiders either.
<BtbN>
He's always tethering on the line of saying anything actually actionable. Banning or removing someone for really liking his ML is not right either
<BtbN>
he's just alone with that opinion and can either adapt or not
<Sean_McG>
BtbN: I think we might have had this conversation before, but what are my options for AArch64 CI node? If I go out and buy a M4 Mac Mini, can I run Linux in a VM on it?
<sfan5>
didn't mean this specific case so much, more in general
<BtbN>
Sean_McG: just use GitHub like we do :D
<sfan5>
Sean_McG: if you have the resources renting an ARM VM at Hetzner is more straightforward
<BtbN>
Sean_McG: you imo don't need to attach runners for your own personal FFmpeg repo
<BtbN>
just open PRs and use the main ones runners
<Sean_McG>
BtbN: for working branches I've always preferred it, just to remove that notion of "works on my machine"
<beastd>
Nah, don't just count Nicolas out! Comeback is not impossible. And he has done good and significant work in the past. But yes the atmosphere here and on the ML is not really welcoming. Especially not for newcomers :-/
<BtbN>
beastd: I mean, Nicolas has at this point directly stated that he hates newcomers and newbies. But someone expects more senior project contibutors to just manifest out of thin air.
<BtbN>
*somehow
<Lynne>
he has said plenty of crap and we have banned him in the past
<beastd>
BtbN: sure I disagree with that. agree with you, kasper93 and others that elitist gate keeping is just not the way
<Lynne>
now he's rejecting everyone's vote, always equating forgejo to a monster and demanding he maintains everything
<Lynne>
like, he's not even in the fucking GA!
<Sean_McG>
that's just toxic
<beastd>
that's silly too (the thing, the monster)
<BtbN>
He also frequently announced that he'll write code for various things, but never follows up
<beastd>
we need more people in ffmpeg for the long game. therefore making it accessible to more and potentially younger devs is good
<Sean_McG>
and I think finally getting a forge is a good step towards that
<BtbN>
Also, the really nice thing about Forgejo is that it's easy to shape it to our needs
<Lynne>
any new dev entering will immediately leave if nicolas gets involved
<BtbN>
that'd be impossible with something highly corporate like Gitlab
<BtbN>
I've already opened multiple PRs with them with stuff FFmpeg could use
<beastd>
although for the lack of maintainers we should probably additionally look into good ways to finance maintainers through a not-for-profit org or sth like that. so it could be funded by companys but they don't have a say in what maintainers do. but i do not know enough about all of this and i have heard often that it is pretty hard in a lot of legal and paper work etc.
<beastd>
regarding Nicolas, despite all the stuff you already criticized above i think he moved a little bit into the right direction in the last days
<sfan5>
this is not personal experience so take this with a grain of salt but I think companies are much more willing to just make donations/grants if you have a legal entity (and not a random person) so they can get their internal paperwork done
<beastd>
Sean_McG: yes I think it could help with a lot of things. also as other mentioned making current maintainers lives easier. all in all working towards a healthier collaboration culture
<beastd>
sfan5: this does not contradict what i just talked about because the not-for-profit org would be a legal entity, right? with legal/paperwork i meant on our side. especially if we would need to found it
<sfan5>
sure, I'm agreeing with you
<beastd>
alright, wasn't sure i understood you correctly
<Lynne>
most companies who want to make donations are AFAIK american
<Lynne>
and these days its a minefield to have a compliant with regulations legal entity
<Lynne>
which can accept american donations
<TheVibeCoder>
and german one
<GewoonLeon>
I think Nicolas' comment about replies to the PR notification on the ML not appearing on the PR is somewhat valid. Maybe if the person replying has an account it sends a comment on the PR via that account and automatically watches the PR and if they don't a bot (e.g. ffmpeg-devel) does it?
<frankplow>
GewoonLeon: I don’t think interoperability of the ML and Forgejo is intended. NG seems to disagree here, but I don’t see anybody else saying this was promised. If the intention is to transition all development to Forgejo, it seems a lost cause to set up so much infrastructure to aid reviewers who refuse to use Forgejo.
<GewoonLeon>
frankplow: that's fair but once you are watching a PR and getting email notifications, you can comment on it by replying to those notifications. I believe currently the only thing that isn't possible is for a person to make their first comment on a PR with just email. Unless you maybe watch the entire repo, but I don't know if that then sends notifications for every single little thing on every single PR; I imagine that'd get old very quick.
<GewoonLeon>
But if watching the repo only gives notifications for PR creations and merges/closing I'd say just tell him to do that
<michaelni>
can someone please review / approve my 3 security related pull requests (20130, 20131, 20134) (please disregard the red X it does not mean a test failure its just a non blocking information)
<jamrial>
michaelni: if no one reviews/approves a pr in like three days, you can push to code.ffmpeg.org and forgejo will sync with it
<jamrial>
and if you make sure the pr has the exact commit hashes, the pr will be marked as manually merged
<averne_>
Hi all, I was wondering if the ffmpeg org had a remote dev server with a gpu attached (amd or nvidia).
<averne_>
I've been developping a prores decoder in vulkan for my gsoc project, which at this point is feature-complete and fairly robust. I'd like to move to optimization but right now I'm away from home until the end of the month and only have access to an intel laptop.
<BtbN>
Don't think so, if purely CLI linux with CUDA is fine for you, I can arrange something though
<BtbN>
I'm actually not sure if it can run vulkan
mkver has quit [Ping timeout: 245 seconds]
<averne_>
I guess CUDA would be fine if nothing else, I'd like to test different strategies for the IDCT and that doesn't need the full decoder to run. But optimally I'd like to look at memory access patterns of the decoder which would need vulkan
<BtbN>
It has a T4 GPU and the full nvidia driver
<BtbN>
just no graphical environment
<BtbN>
so in theory headless vulkan should work, I just never tested it
<BtbN>
DM or mail me your ssh pubkey and I'll set something up
<Lynne>
I had one, but it went dark 12 hours ago
<averne_>
Headless vulkan is no problem, that's what I've been working with at home
<averne_>
Lynne: rip
<TheVibeCoder>
Lynne: bad progamming, kills hardware
<Lynne>
I did ask you a few months ago but you said you had one
<averne_>
What I did was bring my jetson orin devkit with me, which supposedly supports nsight graphics... it just make every app error out after injecting its layer. Unable to create device or whatever, even vulkaninfo doesn't work with it
<BtbN>
like I said, just send me your ssh pubkey
<Lynne>
averne_: if you're benchmarking, really, get an amd
<Lynne>
radeon analyzer is miles ahead, and best of all, it works for pure compute!
<averne_>
BtbN: Will do, thanks
<averne_>
Lynne: I have a 6700xt at home which is my daily driver card, and an rtx3050 that I use in a vm with pcie passthrough (that helps with the amdgpu kmd being pretty bad at TDR). I'm planning to bench on both once I get back
<kasper93>
Yalda: about bd seemless branching. There was always an issue about truehd tracks, having discontinuities in them. Not sure if libbluray handles it well, as is. For more context you can read https://github.com/Nevcairiel/LAVFilters/issues/282https://github.com/domyd/mlp and related discussion on DG Tools forum
<averne_>
Compn: that's kind of you, I already have a desktop at home, not super powerful but good enough for the job. I'm just on holiday this month away from it :)
<averne_>
and my plan for benchmarking fell apart because of the terrible state of nvidia's dev tooling for their jetson platforms
<Lynne>
hahaha, lol
<Sean_McG>
why am I completely unsurprised
BBB has quit [Server closed connection]
BBB has joined #ffmpeg-devel
<frankplow>
Is CODEOWNERS working for others? I didn't get an email or get assigned as reviewer for #20161
<Yalda>
kasper93: thank you, I was not aware of that, I will look
<Sean_McG>
frankplow: I noticed it failed it's fate runs in the CI -- does the CODEOWNERS assignment only take place after that succeeds?
<Sean_McG>
also, Jianhua manually assigned a reviewer
<frankplow>
Sean_McG: Yes I suppose both could explain it. I'm not quite sure what is meant to happen: the CODEOWNERS file simply says it describes which reviewers are "expected". All I want really is to get emails only if the files I listed in CODEOWNERS are touched.
bencoh has quit [Server closed connection]
bencoh has joined #ffmpeg-devel
nto has quit [Server closed connection]
nto has joined #ffmpeg-devel
<Sean_McG>
michaelni: in the absence of someone more qualified, can I send a PR to add myself to CODEOWNERS for **/ppc/* ? I'm not technically a Member yet, unless someone wants to sponsor me in.
<Sean_McG>
if that's a hard requirement, I understand.
<Lynne>
sure
<Lynne>
its just a notification
<Sean_McG>
I suspect it'll be low-volume in my case ^^;
<jianhuaw>
Hi there. May I know how do I upload the files to the fate server?
<BtbN>
You can't upload there yourself
<BtbN>
Generally files should only go there if a patch is imminent to be merged that uses them
<jianhuaw>
Okay. Should I put the download link in the commit message?
<BtbN>
No, before merge, an admin needs to upload it
<BtbN>
Can include the link in the PR/mail about it, and whoever merges it should take care of adding the file before
<jianhuaw>
Okay. I should separate the commits with the fate test to another PR, so the admin can merge them without caring about the others.
<jianhuaw>
BtbN Thank you!
<BtbN>
Normally that's not neccesary
<BtbN>
the splitting I mean
beastd has joined #ffmpeg-devel
<jianhuaw>
Okay. That's fine as well.
<BtbN>
There will be a failing fate test as well (if the code has no external deps at least), so it should be relatively obvious there's a missing sample
<mkver>
kasper93: I intentionally did not use __builtin_assume(), as it does not work with Clang when the condition involves a function: https://godbolt.org/z/5anE6r3oE
<kasper93>
mkver: the same as MSVC's __assume doesn't work in this case
<kasper93>
expecting compiler to expand anything more than simple expressions is bit much
<TheVibeCoder>
how to clone AVIOContext * so if it is used in such a way that when seeiking within such context one get always fixed +offset from original context ?
<kasper93>
I wonder if there is a case where __assume is "stronger"
<mkver>
When I tested this, if (!cond) __builtin_unreachable(); worked wherever __builtin_assume(cond); worked, so I used it.
<mkver>
But when cond contains side-effects (or is not visible to the compiler or so big that the compiler can't infer that there are no side-effects), then this could backfire.
<kasper93>
I'm thinking the same, that on small examples, they would work the same
<kasper93>
but on slightly more complex it migh tickle compiler in different way, after all the __unreachable version depends on compiler to infer the asumption
<kasper93>
instead of giving it directly, at least clang with -O1 converts it to llvm.assume()
<kasper93>
not on O0, but then it doesn't really matter