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

[burek]

[00:01] <cone-893> ffmpeg.git 03Lukasz Marek 07master:f5695926235c: lavd/pulse_audio_enc: fix error check
[00:10] <saste> http://linux-fbdev.sourceforge.net/ -> status
[00:37] <cone-893> ffmpeg.git 03ÞSÊ 07master:4bfdd021c71b: configure: remove \r from cc_ident,
[01:00] <cone-893> ffmpeg.git 03Michael Niedermayer 07master:12e66f205669: MAINTAINERS: add Timothy to documentation
[02:17] <BBB> ubitux: I think it's because the multiplier was 16 bit (i.e. >=32768) so I wasn't sure if it would work correctly (sign-wise) with pmaddwd, which expects signed input
[02:18] <BBB> ubitux: so to fix that, I *2 the input and /2 the other input to have it work out
[02:18] <BBB> since a*x is the same as (a+a)*(x/2) if x is even, or (a+a)*(x/2)+a if x is odd
[02:18] <BBB> I think x was even
[02:19] <BBB> so then it's simple
[03:41] <mathstuf> ok...so i found out where the STRINGS_METADATA goes when a codec attaches it
[03:42] <mathstuf> (av_packet_merge_side_data in avpacket.c)
[03:43] <mathstuf> though i dont see how the app is supposed to get that data back easily...
[03:43] <mathstuf> i guess test the last 64 bits for FF_MERGE_MARKER?
[03:44] <mathstuf> ah, av_packet_split_data
[04:40] <sirloin1> Hi guys I think I stumbled upon a bug in ffmpeg, but I was wondering if I could get someone's opinion. It has to do with RTSP session ID. I noticed it doesn't seem to handle the full charset from the RFC 
[04:41] <sirloin1> oops not a ffmpeg bug i think... sorry
[11:13] <cone-239> ffmpeg.git 03Luca Barbato 07master:2b72f8ac320c: wtv: Seek by sector properly
[11:13] <cone-239> ffmpeg.git 03Michael Niedermayer 07master:5d2a83571b0a: avformat/wtv: rename to wtv_common
[11:13] <cone-239> ffmpeg.git 03Michael Niedermayer 07master:2a5f4c1ea2c7: Merge remote-tracking branch 'qatar/master'
[11:41] <cone-239> ffmpeg.git 03Michael Niedermayer 07master:fec6d547cc27: avformat/wtvdec: drop SHIFT_SECTOR_BITS() macro
[12:37] <ubitux> michaelni_: i though the purpose of this was the int64_t cast
[12:37] <ubitux> otherwise it's 32-bit wide and fails with big files
[12:38] <michaelni> ubitux, yes 
[12:38] <ubitux> but i guess 1<<12 is ok, and sector is already int64 mmh
[12:50] <wm4> why do some source files have an av prefix, and some don't? is this just a general inconsistency?
[15:14] <Daemon404> michaelni, ping pthread patch when you have time
[15:20] <cone-239> ffmpeg.git 03Michael Niedermayer 07master:a6388616e826: doc/developer: Add a policy item about updating the MAINTAINERs file
[16:49] <cone-239> ffmpeg.git 03Michael Niedermayer 07master:d5ec8ba7f2cb: Do not leave positive values undefined when negative are defined as error
[16:52] <ubitux> this will break some applications randomly if you start changing the behaviour
[17:04] <wm4> ubitux: does packet side data need to be verified or not?
[17:05] <wm4> tried to get an answer about this out of the Libav devs, but couldn't get a clear answer
[17:05] <ubitux> verified?
[17:05] <ubitux> verify what?
[17:07] <wm4> the contents... my question is whether side data is always internally generated and stored (so you can be sure it's in the form it's required to be), or if it could come from external files and has to be checked in order to avoid security issues
[17:09] <ubitux> side data content is sometimes extracted as "raw" from the input
[17:09] <ubitux> typically identifier or settings from webvtt
[17:09] <wm4> which means?
[17:09] <wm4> so it depends on the side data type?
[17:09] <ubitux> so if you plan to printf(side_data), i could generated a webvtt with a "%s" in it and make you crash
[17:10] <wm4> so it's mixed?
[17:10] <wm4> what about AV_PKT_DATA_STRINGS_METADATA specifically?
[17:10] <ubitux> likely controlable by the user as well
[17:11] <ubitux> some side data should be safe, but not all of them
[17:11] <wm4> comical
[17:11] <ubitux> but it should be properly formatted
[17:11] <ubitux> if that's what you mean by verifying
[17:12] <ubitux> there should be no direct control on the layout by the user
[17:12] <wm4> user=? API user, application user?
[17:12] <ubitux> application user
[17:12] <ubitux> for api user i would guess they can do whatever they want anyway
[17:13] <ubitux> and i was thinking of crafted files
[17:13] <wm4> the question is whether they're supposed to...
[17:13] <ubitux> i don't think an input file can cause the generation of a side data not following the layout
[17:13] <ubitux> but note that in our code base we actually do test if the size is the expected one for instance
[17:14] <wm4> another question, why is side data not just memcpy() of a struct?
[17:14] <wm4> (in general, of course it wouldn't work with AV_PKT_DATA_STRINGS_METADATA)
[17:14] <ubitux> endianess maybe
[17:14] <wm4> lu_zero has been saying something about serializing
[17:14] <wm4> different endianess on the same machine, same process?
[17:15] <wm4> while we're at it, what does av_packet_get_side_data do? it's undcoumented
[17:15] <ubitux> what if we avio_write the side data to an output file
[17:15] <wm4> man
[17:15] <ubitux> (in a muxer context)
[17:15] <wm4> so what if you avio_read it
[17:16] <wm4> do you verify it after reading?
[17:16] <wm4> or does the API user have to do that?
[17:16] <ubitux> i can't tell from memory
[17:16] <wm4> and if it's per side-data, it should be documented
[17:17] <wm4> well, _especially_ in this case
[17:20] <wm4> I still don't get this side data split/merge stuff
[17:20] <wm4> must be something completely hysteric
[17:21] <wm4> if the packet data contains a magic value (FF_MERGE_MARKER) at some random looking position, the packet is considered to contain side data?
[17:21] <wm4> if (!pkt->side_data_elems && pkt->size >12 && AV_RB64(pkt->data + pkt->size - 8) == FF_MERGE_MARKER){
[17:22] <wm4> (so, at the end of the packet)
[17:24] <wm4> and whether to use this shit is decided by the "keepside" avoption flag
[17:24] <wm4> wat
[17:28] <wm4> ok so it seems libavcodec always calls av_packet_split_side_data() on each packet
[17:28] <wm4> so at least libavcodec itself has to verify side data always
[17:29] <michaelni> ubitux, do you suggest that master:d5ec8ba7f2cb should be reverted or something else or i misunderstood ?
[17:30] <wm4> and I found the commit which added this shit: 94eadee7efc2c5d19ecfe92d36f0556663468080
[17:30] <wm4> with no explanation whatsoever
[17:30] <wm4> so, does anyone even know why av_packet_split_side_data() is called?
[17:30] <wm4> and I fully expect the answer to be "nut"
[17:31] <wm4> while possibly subtly breaking some other formats, or adding security issues
[17:31] <wm4> unless I'm hopefully misunderstanding this
[17:32] <nevcairiel> the only possible  breakage would be if some format contains this 64-bit marker at a specific position, right?
[17:33] <wm4> nevcairiel: at the end of the packet
[17:33] <michaelni> software that isnt avcodec based likely doesnt use AVPackets and alot of it only transports data/size from demuxer to decoder, encoder to muxer, demuxer to muxer, so putting sidedata in data simplifies integration alot for such apps
[17:33] <wm4> #define FF_MERGE_MARKER 0x8c4d9d108e25e9feULL
[17:34] <wm4> so that software is supposed to use av_packet_merge_side_data() on the demuxer side?
[17:34] <michaelni> also it should allow generic containers to preserve side data if they just store and load packets
[17:34] <nevcairiel> hitting exactly that 64-bit marker seems rather unlikely, but i agree that its a terribly ugly thing to do, especially if you only use avformat and not avcodec, you can end up with junk data in your packets that you feed to external decoders - if you don't disable it
[17:34] <wm4> ok so my conclusion of this mess is: be as defensive as possible when touching side data
[17:35] <michaelni> apps that dont want this merge stuff can set AVFMT_FLAG_KEEP_SIDE_DATA
[17:36] <wm4> isn't that default
[17:37] <michaelni> no
[17:37] <Daemon404> hey nevcairiel is bac
[17:39] <wm4> so is this a security bug? http://git.videolan.org/?p=ffmpeg.git;a=blob;f=libavcodec/utils.c;h=3832b816770ae47bd17667eccf6a9db417dac5e3;hb=HEAD#l1944
[17:39] <wm4> it basically calls strlen() on packet data
[17:40] <wm4> you'd just have to prepare an evil file with FF_MERGE_MARKER at the end of packet data, and then you can at least get ffmpeg to crash?
[17:40] <wm4> or am I getting this wrong
[17:44] <michaelni> any demuxer that supports loading generic side data would allow the same, you dont need FF_MERGE_MARKER, but yes this code looks problematic
[17:45] <michaelni> side data should not be trusted more than data
[18:00] <cone-239> ffmpeg.git 03Michael Niedermayer 07master:240fd8c96f59: avcodec/avpacket/av_packet_split_side_data: ensure that side data padding is initialized
[18:00] <cone-239> ffmpeg.git 03Michael Niedermayer 07master:838f461b0716: avcodec/utils: add some saftey checks to add_metadata_from_side_data()
[18:08] <nevcairiel> Daemon404: kinda, still need to sleep like 16 hours to recover from the flight =P
[18:32] <kierank> nevcairiel: are you at gsoc con as well?
[19:10] <ubitux> michaelni: no i'm not saying it should be reverted since it should not be wrong currently, but i think it's risky because if you are more likely to change the behaviour ("it's ok since it now matches the doc")
[19:17] <michaelni> yes, what do you suggest ?
[19:22] <ubitux> being careful not to change the behaviour
[19:22] <ubitux> even if it matches the doc&
[19:23] <beastd> at least for new functions we should always document it that way (regardless of the behaviour if the function only returns 0 for success at that time)
[19:23] <ubitux> sure
[19:23] <beastd> libav* callers should not check the result of those kind of functions with if (rc == 0)...
[19:24] <beastd> ubitux: ack, to being careful to not change functions that were (initially) documented that way
[19:27] <beastd> if the desire arises to change one of those we should probably add a new symbol and retain the old one with the return-0-on-success behaviour
[19:48] <kierank> Daemon404: lol vp9 presenting at smpte
[19:59] <ubitux> smjd: ping
[19:59] <smjd> o_o
[19:59] <smjd> pong
[19:59] <ubitux> smjd: i looked at the gif thing
[19:59] <ubitux> are you the guy from the ticket?
[19:59] <smjd> no
[19:59] <ubitux> ok
[20:00] <ubitux> anyway, i have a working patch here
[20:00] <ubitux> but i wonder how you can have correct timing with that method
[20:00] <ubitux> (even before the changes)
[20:01] <ubitux> well anyway, i'll send a patch soon so you can test if you are interested
[20:02] <smjd> I can test it, but I don't need to do any GIF stuff now
[20:02] <smjd> does it need testing?
[20:03] <smjd> by now, I mean at the moment
[20:03] <ubitux> probably, dunno
[20:04] <ubitux> "soon" :p
[20:04] <ubitux> smjd: how do you set the timing with gif sicle?
[20:04] <ubitux> do you know if it's using the delay at the end of each gif when available?
[20:07] <smjd> ubitux: the man page says --delay <ms>
[20:09] <ubitux> smjd: http://b.pkh.me/0001-avformat-image2-allow-muxing-gif-files.patch
[20:09] <ubitux> feel free to try this out
[20:09] <ubitux> still a WIP, i need to do proper testing (it's mostly untested currently)
[20:10] <ubitux> ./ffmpeg -i <input> -frames:v 150 -c:v gif -f image2 'out%d.gif'
[20:15] <smjd> I'll test it in a few hours
[20:16] <BBB> ubitux: how's 4x4idct for vp9?
[20:17] <ubitux> BBB: didn't progress today, this gif thing is considered a regression (of which i'm responsible)
[20:52] <ubitux> ok, back to vp9 now :p
[21:40] <Daemon404> kierank, fun
[23:01] <cone-239> ffmpeg.git 03Michael Niedermayer 07master:6838e1f547d4: avformat/oggdec: favor av_freep() over av_free()
[23:24] <michaelni> ubitux, want to take a look at CID1108581 (ffprobe potential strchr(NULL)) ?
[23:25] <ubitux> later maybe, i really want to get done with vp9
[23:26] <michaelni> sure no hurry, ill ask sate or do it myself
[23:33] <cone-239> ffmpeg.git 03Michael Niedermayer 07master:d0812f91c810: avcodec/exr: favor av_freep() over av_free() for saftey
[23:33] <cone-239> ffmpeg.git 03Michael Niedermayer 07master:80b1e1c03d26: avcodec/exr: fix null pointer dereference
[00:00] --- Sun Oct 20 2013