[Ffmpeg-devel-irc] ffmpeg-devel.log.20180513

[burek]

[00:30:12 CEST] <jkqxz> Who maintains the doxygen stuff on ffmpeg.org?  The doxygen documentation for the 4.0 branch is missing - I guess that needs a manual step to add a branch somewhere?
[00:32:23 CEST] <atomnuker> michaelni I think
[01:05:50 CEST] <michaelni> doxygen for 4.0 should be fixed
[01:13:24 CEST] <jkqxz> Yep, working now.  Thank you!
[09:21:47 CEST] <KTSamy> Even though it's not an issue, Just asking on curiosity.May I know why RELEASE file is not updated after 4.0 release. It still "3.4.git".
[09:25:06 CEST] <KTSamy> when using the latest builds from the master branch, it always shows as 3.4.git which results in confusions sometimes 😂
[11:19:16 CEST] <JEEB> &23
[11:27:31 CEST] <BtbN> Because nobody thought update updating it so far
[11:28:21 CEST] <JEEB> michaelni: I am very confused about this FLV patch and you saying LGTM to it
[11:29:00 CEST] <JEEB> I mean, it could be that nobody cares and I shouldn't either, but (unsigned)pkt->dts and the function definition being "unsigned" just makes me cringe
[11:30:04 CEST] <JEEB> also FLV had timestamp wrap-arounds and those are ignored completely in this fix even though figuring out if it's 32bit or 31bit shouldn't be too hard. most likely 31bit wrap-around since the spec says "starts at zero" and the value is 32bit signed integer
[11:33:15 CEST] <JEEB> and no, I haven't looked outside of the context of that patch how timestamp wrap-arounds are handled in flvenc.c. maybe they aren't. in which case it might feel silly that I'm trying to require these things, but so far nobody has mentioend that.
[11:33:53 CEST] <JEEB> I'm just going by the anecdotal info I have of FLV, and the thing Adobe calls a "spec"
[11:34:52 CEST] <BtbN> iirc flv/rtmp just completely shit itself when the timestamp overflows
[11:37:20 CEST] <JEEB> the timestamp value seems to be a signed 32bit integer (supposed to start at zero according to spec, but apparently in RTMP this initial timestamp can be nonzero according to alex?), and the time base is 1/1000. so it seems like it's 31bit unsigned integer wrap-around?
[11:37:31 CEST] <JEEB> at least that's how the signed integer seems to be realistically utilized
[11:37:49 CEST] <JEEB> someone splunked an "int" and then only used it for positive values
[11:37:58 CEST] <JEEB> but someone would have to look at how adobe crap handles that
[11:38:56 CEST] <nevcairiel> thats still like almost 600 hours at that granularity, not sure that realistically matters
[11:39:09 CEST] <BtbN> all the streaming services kill your stream after ~48 hours
[11:39:38 CEST] <durandal_1707> why?
[11:42:03 CEST] <nevcairiel> the real question is, how are normal packet timestamps written? and is this different? if no, then its perfectly fine
[11:42:48 CEST] <JEEB> as in, "if we have these issues already there, then we can make more code with the same issues?"
[11:42:55 CEST] <nevcairiel> yes
[11:43:07 CEST] <nevcairiel> asking people to fix pre-existing issues in a patch is generally not very nice
[11:43:28 CEST] <nevcairiel> that should be separate work
[11:44:12 CEST] <JEEB> and I brought up things because f.ex. he now specifically has a zero as the function parameter for the initial data, thus it feels like it's being asserted that that's the correct thing now
[11:44:26 CEST] <JEEB> also I wish these things were mentioned as replies to my reviews
[11:44:34 CEST] <JEEB> - we will not handle wrap-around correct because <reason>
[11:44:43 CEST] <JEEB> - we will not handle other things correctly because <reason>
[11:45:14 CEST] <nevcairiel> what other timestamp would it use in the header? there is none to be had there =p
[11:45:50 CEST] <JEEB> also my general gist is that new code should be correct. I would not make him fix normal timestamp writing code, but if he's fixing the header update packets' timestamps then might as well figure out the correct way to do that
[11:46:27 CEST] <JEEB> nevcairiel: fair point if that was in write_header
[11:46:35 CEST] <nevcairiel> it is
[11:46:51 CEST] <JEEB> that said I think I stopped talking about that thing on the ML some time ago
[11:47:11 CEST] <JEEB> and mostly wanted to focus on finally getting the timestamps right in that specific case
[11:47:35 CEST] <nevcairiel> the way I see it, the timestamps should match the way timestamps are written for normal packets
[11:47:42 CEST] <nevcairiel> if they are different, its going to cause issues
[11:47:47 CEST] <nevcairiel> no matter how right or wrong that method is
[11:48:22 CEST] <nevcairiel> getting a packet and a header update with different timestamps even if they belogn together is clearly worse then anything else that could happen
[11:49:23 CEST] <JEEB> and as far as I can see that's relatively unlikely unless the normal timestamp code does something really wonky
[11:53:44 CEST] <JEEB> but sure, if it is mentioned that all these other technical concerns about "how does FLV actually work" are ignoed, then there's the nr1 issue that throws itself at my eyes which is the aversion of standardly-sized types.
[11:54:12 CEST] <JEEB> we might have taken code that does (unsigned)int64_t before, but I'm pretty sure we don't any more
[11:58:30 CEST] <JEEB> and seriously, maybe I should just give up at this point. my original reasoning was that I wanted to provide a nice review so that alex's patch didn't get ignored on the mailing list - with the information on FLV I had available. and now it seems like everyone thinks that I'm the bad person being a pedantic asshole. even though I've mentioned a few times that the change is good and as far as we 
[11:58:36 CEST] <JEEB> understand how FLV works the replies to my reviews should have been a simple case
[12:00:12 CEST] <JEEB> and I feel even more sadder that I now had to quickly/hastily do a reply on the new version of the patch that just does (unsigned)int64_t for the function about the most glaring issue with it. and now I feel awful about that too. fuck life
[12:00:52 CEST] <JEEB> because michaelni LGTM'd the patch with that thing
[12:04:28 CEST] <michaelni> JEEB, i dont understand your concerns, the patch wirtes the timestamp the same way as other timestamps are written in flvenc. If this has issues that must be fixed and is very important but it should not be in this patch. it should either be before or afterwards
[12:04:33 CEST] <michaelni> thats IMHO
[12:05:51 CEST] <JEEB> ok, even if we ignore all theo ther concerns
[12:06:00 CEST] <JEEB> is (unsigned)int64_t OK in new code?
[12:06:34 CEST] <JEEB> like, that is the only left-over thing if we ignore all of the "how FLV actually works" discussion out of this
[12:07:06 CEST] <JEEB> we're adding a *new* function parameter and that's now "unsigned", and the value we're trying to stuff in is int64_t
[12:07:22 CEST] <JEEB> since it's a new and not public thing, why is everyone just OK with that?!
[12:07:42 CEST] <JEEB> I know for external APIs you might have to do non-pretty things
[12:07:51 CEST] <michaelni> int64_t -> unsigend is how existing timestamps are done in flvenc. If we change it it should be changed for all at once.
[12:09:47 CEST] <JEEB> but this is the important part: we are now letting an example of new code in (albeit in an old module) with something that we generally would not accept
[12:10:04 CEST] <JEEB> are we really OK with that?
[12:10:20 CEST] <JEEB> and how different is it from just taking those 32 or 31 bits within that function from that int64_t?
[12:10:37 CEST] <JEEB> but OK, clearly I don't understand anything and I should just shut up
[12:10:56 CEST] <michaelni> shift & mask should be the same for int64 and unsigned
[12:12:11 CEST] <michaelni> about letting bad code in, we should not. But i dont think flvenc authors considered >2^31 timestamps very deeply
[12:12:36 CEST] <michaelni> this should be fixed but one fix has to be done first
[12:13:50 CEST] <michaelni> fix the 0 timestamp as in the patch or fix any 32 / 31 bit issues if they exist
[12:13:55 CEST] <JEEB> like, I am still boggled about this logic but I will just post "Fuck it, ignore all of my concerns, seemingly I'm the bad guy here." on the ML I guess
[12:14:25 CEST] <JEEB> because it seems like most of the community has differing standards than I have, and I do not deal with FLV daily and thus cannot take this fixing forward by myself
[12:16:11 CEST] <durandal_1707> it is broken before, it is broken after...
[12:19:27 CEST] <michaelni> JEEB, if you suggest that the patches should use int64 for all new timestamp variables instead of unsigend but generate exactly the same file that seems possible. But if you want to write different timestamps in one field than others thats adding a worse issue IMHO
[12:20:14 CEST] <JEEB> I just fucking saw one function that had a parameter added to it for eff's sake
[12:20:27 CEST] <JEEB> and that parameter was unsigned while the data it takes in was clearly int64_t only
[12:20:43 CEST] <JEEB> that was litearlly the only other concern I had of that patch outside of "how does FLV work"
[12:20:57 CEST] <JEEB> (in the area that is not well specified in the spec)
[12:23:54 CEST] <JEEB> there, I thus withdrew from that review since I'm seemingly the asshole.
[12:30:36 CEST] <wm4> seems ffmpeg can't decide whether changes to a patch require v2, or if "changing before pushing" is ok
[12:30:46 CEST] <wm4> I conclude anything goes
[12:31:21 CEST] <michaelni> wm4, thats handled inconsistently it seems yes
[12:32:27 CEST] <JEEB> but yea, I think I will just sulk somewhere because I honestly was trying to be useful.
[12:33:25 CEST] <michaelni> JEEB, i think your good intentions overshoot the goal but maybe i just misunderstand which is very possible
[12:34:16 CEST] <michaelni> but ive a patch locally that fixes a unsigend/int64 bug in flvenc ill post once i tested a bit
[12:34:23 CEST] <JEEB> all I wanted was to figure out how FLV actually handles that, not actually fixing the whole thing. to have this as an example of how to do it right.
[12:34:36 CEST] <michaelni> never would have found this bug that patch fixes without you :)
[12:36:52 CEST] <JEEB> and yes, I am myself very guilty of not looking at the rest of the code of flvenc.c outside of that patch. but even with that, I think my questions regarding how these things are handled were valid since we were adding a new place where FLV timestamps were being added
[12:37:30 CEST] <JEEB> or well, a new place which writes FLV timestamps. period.
[16:59:43 CEST] <hanna> https://0x0.st/sjFc.txt trying to enable lv2 support fails for me -> it can't find /usr/include/lilv-0/lilv/lilv.h, since that doesn't exist. on my end the package installs to /usr/include/lilv/lilv.h
[16:59:56 CEST] <hanna> It seems like the ffmpeg include should also be using #include <lilv/lilv.h>
[17:00:07 CEST] <hanna> after all, on platforms where /usr/include/lilv-0 is needed, the pkg-config --cflags seems to include that
[17:00:38 CEST] <JEEB> yea, that looks weird as far as a check is concerned
[17:00:51 CEST] <JEEB> possibly a historical thing
[17:00:59 CEST] <hanna> in fact it's just the configure check that needs fixing
[17:00:59 CEST] <JEEB> time for archaeology
[17:01:02 CEST] <JEEB> yes
[17:01:04 CEST] <hanna> the actual source code uses <lilv/lilv.h>
[17:01:08 CEST] <JEEB> lol
[17:01:40 CEST] Action: JEEB waits for the historical context to pop up
[17:02:12 CEST] <JEEB> basically I 100% agree, but am not sure if someone comes up with some weird case where that's somehow needed
[17:02:36 CEST] <JEEB> ok, it was just added as a component by paul
[17:02:42 CEST] <JEEB> and it's a pkg-config check
[17:02:58 CEST] <JEEB> and it probably passed because both were there
[17:03:03 CEST] <hanna> https://0x0.st/sjFT.patch if you want a patch :p
[17:03:06 CEST] <JEEB> the -I flag from pc
[17:03:12 CEST] <JEEB> well yea, the patch is pretty obvious
[17:03:20 CEST] <JEEB> since I'm looking at the check
[17:03:20 CEST] <JEEB> lol
[17:04:05 CEST] <JEEB> but yea, it worked in the pkg-config check because both the include dir as well as the extra lil-related dir were in -I
[17:04:12 CEST] <hanna> yeah but this way you just have to curl | git am && git push :p
[17:04:12 CEST] <JEEB> and the other one as well worked because were there
[17:04:35 CEST] <JEEB> true
[17:05:05 CEST] Action: JEEB pokes durandal_1707 to verify that his system still works with that patch
[17:05:14 CEST] <JEEB> if it works for him (it most likely does), then it can be pushed
[17:05:28 CEST] <JEEB> or well, officially it needs to go through ML etc
[17:05:28 CEST] <hanna> (plus I found the bug and required the change, shouldn't I be the one to attach my name to the fix so that people can complain? :p)
[17:05:36 CEST] <JEEB> yes
[17:05:52 CEST] <hanna> fuck mailing lists for one-word changes
[17:05:58 CEST] <hanna> but I can send it if you really want to
[17:06:00 CEST] <JEEB> I know, but that's the process
[17:06:03 CEST] <JEEB> yes, I will send it right now
[17:06:33 CEST] <hanna> I could also just type `git send`
[17:06:47 CEST] <hanna> unless you already sent
[17:06:56 CEST] <hanna> I didn't realize I already had git send configured for ffmpeg
[17:07:04 CEST] <JEEB> nah, I just grabbed my laptop that has git send-email configured
[17:07:05 CEST] <JEEB> lol
[17:07:20 CEST] <JEEB> so if you can send it, pls do
[17:07:26 CEST] <JEEB> then I will poke durandal_1707 do LGTM it
[17:07:28 CEST] <JEEB> if it works for him
[17:07:35 CEST] <JEEB> since he's the only other person with that library
[17:07:42 CEST] <JEEB> (known)
[17:07:49 CEST] <JEEB> since otherwise the breakage would have been noticed
[17:09:53 CEST] <hanna> well it might be that gentoo is wrong in installing it directly to /usr/include/lilv-0
[17:10:01 CEST] <hanna> although that just seems to be the default behavior of the thing if you pass --prefix to the waf script
[17:10:05 CEST] <hanna> I mean directly to /usr/include
[17:11:14 CEST] <JEEB> CC'd paul
[17:11:21 CEST] <JEEB> in my "looks good" reply
[17:14:57 CEST] <hanna> FWIW, the only way that could possibly break if is there exists a platform that installs it to /usr/include/lilv-0/lilv-0/lilv/lilv.h
[17:15:02 CEST] <hanna> but that seems too retarded for anything other than debian
[17:15:20 CEST] <hanna> oh, and in that case, the configure check would work but the af_lv2.c would fail compiling
[17:15:36 CEST] <hanna> really there's no possible case in which having the configure check mismatch the actual source code would either be needed or beneficial
[17:15:44 CEST] <JEEB> yea
[17:15:46 CEST] <JEEB> I 100% agree
[17:16:08 CEST] <JEEB> for teh record, that 0x0.st link hasn't still opened for me btw
[17:16:08 CEST] <JEEB> lol
[17:16:16 CEST] <JEEB> so I never could even try or apply it
[17:16:26 CEST] <JEEB> (although I could have done the change locally)
[17:16:35 CEST] <hanna> curl https://0x0.st/sjFT.patch doesn't work for you?
[17:16:51 CEST] <JEEB> it works now
[17:16:57 CEST] <hanna> were you trying to open it in a browser?
[17:17:07 CEST] <JEEB> and still did, which works generally OK
[17:17:18 CEST] <JEEB> but yes, I do curl + git am generally
[17:17:22 CEST] <JEEB> after checking the patch visually quickly
[17:17:42 CEST] <JEEB> I just seem to have accessed 0x0.st exactly when something borked somewhere between here and the server
[17:18:25 CEST] <hanna> I blame lachs0r
[17:18:27 CEST] <hanna> (who isn't here, wtf)
[17:54:14 CEST] <JEEB> alright, paul lgtm'd it
[17:56:40 CEST] <JEEB> will quickly check that configure script didn't break and then push it in
[17:56:46 CEST] <JEEB> I mean, it most likely didn't break
[17:56:48 CEST] <JEEB> but you know
[17:57:02 CEST] <JEEB> I want to be able to answer "did you even try running the shell script?!"
[17:57:38 CEST] <hanna> I mean I built it using this patch locally already
[17:57:42 CEST] <JEEB> ok, works
[17:57:48 CEST] <JEEB> hanna: yea this is just a general workflow thing
[17:57:50 CEST] <hanna> (which also confirms that it solves my issue)
[17:58:03 CEST] <JEEB> I don't generally push things that I have not even had in my tree
[17:58:07 CEST] <JEEB> and configured+built at least once
[17:58:12 CEST] <JEEB> even if that feature is not enabled
[17:58:29 CEST] <hanna> as if changing the contents of a string would break the configure script :p
[17:58:41 CEST] <hanna> wait fuck, this is shell, everything's a string
[17:58:57 CEST] <cone-890> ffmpeg 03Niklas Haas 07master:32234e03a792: configure: fix configure check for lilv-0
[17:59:01 CEST] <hanna> \o/
[17:59:02 CEST] <JEEB> there we go
[17:59:13 CEST] <JEEB> that was also part of ffmpeg 4.0 as well, so maybe needs back-porting
[18:00:09 CEST] <hanna> how many layers of bureaucratic BS does that need to go through?
[18:00:21 CEST] <JEEB> someone saying "maybe" or "yea"
[18:00:25 CEST] <JEEB> and then I just push it in
[18:00:31 CEST] <JEEB> or someone else does
[18:00:38 CEST] <jamrial> hanna: none, just making sure it works
[18:00:41 CEST] <hanna> I remember trying to submit a trivial bug fix to gentoo and they insisted I provide like 10 different log files that had fuck all to do with the typo fix
[18:00:46 CEST] <hanna> because their policy requires it
[18:00:52 CEST] <hanna> at some point I just gave up
[18:01:08 CEST] <JEEB> yes, if you are brave enough you just push. but I like to do the minimum
[18:01:14 CEST] <JEEB> like actually running the shell script/build
[18:01:20 CEST] <JEEB> for a patch that I am about to push
[18:02:08 CEST] <jamrial> workflow is pretty lax. if it works, it's not spaghetti code and no one blocks it or is against, in general it's fine
[18:05:09 CEST] <JEEB> which of course has its positives and negatives. generally I like to follow: 1) patch onto ML 2) at least someone else has taken a look at the patch, preferably the person who maintains that module 3) check that the patch hasn't broken anything obvious
[18:05:59 CEST] <JEEB> I know that's not required, but it helps to have a distinct workflow :P I'm 100% OK with stuff like that documentation fixup which tmm1 did and just pushed, although someone could possibly argue that it's wrong because it didn't go through the ML
[18:19:32 CEST] <cone-890> ffmpeg 03Niklas Haas 07release/4.0:93cee87b13fd: configure: fix configure check for lilv-0
[18:19:38 CEST] <JEEB> and there is a back-port
[18:53:19 CEST] <KTSamy> foe me includes goes to lilv-o/lilv
[18:53:38 CEST] <KTSamy> in wscript it's includedir = '${INCLUDEDIR}/lilv-%s/lilv' % LILV_MAJOR_VERSION
[18:54:05 CEST] <KTSamy> I have indeed used --prefix to point a custom directory
[18:54:10 CEST] <JEEB> yea, but the check was pkg-config based and the pc file should have the required -I
[18:54:25 CEST] <KTSamy> Yes. It has
[18:54:26 CEST] <JEEB> also the code didn't use the lilv-X prefix
[18:54:44 CEST] <JEEB> the include in the .c file had only lilv/blah.h
[18:54:46 CEST] <JEEB> :)
[18:55:03 CEST] <KTSamy> I have tried ffmpeg master on mac
[18:55:12 CEST] <KTSamy> everything went fine
[18:55:27 CEST] <JEEB> yup, if it wouldn't have worked the code wouldn't have compiled in the first place
[18:55:35 CEST] <JEEB> since the header check had a different path
[18:55:59 CEST] <KTSamy> my pkg-config pc file has the required -I
[18:57:12 CEST] <KTSamy> my HEAD is at cae004cabb
[18:57:27 CEST] <KTSamy> that doesn't include the last fix for lilv
[22:21:57 CEST] <AndroidDeveloper> Hello! Am I allowed to use binary file (static linking, compiled with --enable-static) in Android closed-source application (apk) and provide user an option to set its own path to any FFmpeg binary file he wants (to use it instead of mine)?
[22:24:10 CEST] <JEEB> with LGPL I see that as long as you give the user the source code you used and the possibility to switch it "should" be OK. With GPL if it only works with those GPL components then you can't keep your thing only closed source since I'd consider your application be clearly requiring GPL software.
[22:24:16 CEST] <JEEB> but IANAL
[22:24:37 CEST] <JEEB> AndroidDeveloper: and you're just calling ffmpeg.c and not linking against the libraries?
[00:00:00 CEST] --- Mon May 14 2018