GNOME Bugzilla – Bug 601236
[flvmux] script tag with index gets written at end of file, contains all tags
Last modified: 2010-03-15 12:57:39 UTC
This pipeline in gst git: gst-launch-0.10 videotestsrc num-buffers=10 ! ffenc_flv ! flvmux name=mux ! filesink location=/tmp/test.flv audiotestsrc num-buffers=10 ! lame ! mux. produces a file that has two script tags, one at the beginning: #00001 <ScriptTag onMetaData at offset 0x0000000D, time 0, size 99> {'metadatacreator': 'GStreamer FLV muxer', 'creationdate': 'Mon Nov 9 9:52:19 2009'} and one at the end: #00021 <ScriptTag onMetaData at offset 0x0001123A, time 0, size 392> {'times': [0.0, 0.0, 33.333333, ( ...snip... ) 300.0], 'filepositions': [12679.0, 13091.0, 20870.0, ( ...snip... ) 70202.0]} The last tag gets written by gst_flv_mux_write_index, which gets called on EOS. The index contains timestamps of all FLV tags in the muxed file and their byte offsets. There are two things wrong here: the index should not be written at the end of the file, since this is not useful. Many tools and video players written in ActionScript assume that the script tag with the index will be the first tag in a FLV file and use the information from that index as a seek table. Putting that index at the end of the file is useless. Second, the index should not contain all tags times and positions, only key unit's. Since players use that info as a seek table, putting all tags' there will only confuse them. Usually, that index in FLV files is added by external tools, like Flvtool2 or flvlib. If GStreamer writes an index to the file, it should produce output similar to what you'd get using these tools.
If the flv container format allows for padding/void blobs, then the muxer could be modified to write such a blob at the beginning, then on EOS seek back to that location and write the proper information. One problem though... is that you're not sure how much size you'll need to write the index. One could do a smart guess-timate based on incoming framerate and duration and then when writing, try to put as much information as possible. There's a reason why those indexes are written using separate tools, it's because you need to allocate enough (or better exactly) the amount of bytes needed to write that index. It's a problem similar to creating "fast-start" qt/mov/iso files where the header atom is located at the beginning.
Yeah, it's exactly like fast-starting qt files. WRT to padding in FLV, you could try to cheat by frist writing a large blob at the beginning, then computing the index values, then seeking, putting the values there and filling the rest of the script tag with a string value of "filling": "ROAAAAAA(...)AARGH", with as much "A"s as necessary. That would still require guessing the initial blob size, though. Alternatively, flvmux could just rewrite the whole output after EOS (which is what qtmux does IIRC if you specify the faststart property).
So I have a preliminary patch that reserves enough space in the metadata to write 128 index entries (the current code never writes more than 128 index entries, so it seemed like a good constant). 128 keyframes is not a lot, but if you don't want to rewrite the whole file, you need to put the limit somewhere. After eos, the muxer would seek to the appropriate position in the metadata (by sending a BYTES new segment - logic copied from avimux), write the index and fill in the remaining space (if there's any) with a filler, like a "ROAAA...RGH" string entry, in the onMetaData script tag. The fact the in my current patch it *is* using that string is one of the reasons I'm not attaching it yet ;) Anyone has a better idea for a sciprt tag filler? There's a problem that if downstream doesn't accept the new segment (think live streaming), the consumer will see a giant ROAAARGH property in the metadata. Of course this can be thought of as acceptable (and the muxer could get a property disabling any index writing logic whatsoever). All that's a bit kludgy, I'm not sure if it's worth it at all if there are numerous 3rd party tools to index FLV files. I would go as far as suggest to make the property that disables index writing default to true and if someone really wants to have an index (with at most 128 entries!) and a silly filler string written out by gst, let them enable it.
You could add a script tag of some undefined type instead of a string script tag... demuxers probably ignore those ;)
My FLV verificatino library errors out on invalid script values, so I'd rather avoit that ;) Also, I'd be worried about demuxers ignoring the whole tag when seeing something invalid. A string filler has the nice property of being able to make it as long/short as necessary. There's also the option to put a filler with the type "MovieClip" (0x04), or just nest arrays until you fill the required number of bytes ;) I'd go for a filler called "gstprivate" and make it contain just spaces. For files it will usually not be there, anyway, unless they have < 128 keyframes.
Finally able to get back to this. I have a set of patches that fix the two things mentioned in this bug (index contains all tags and index gets written at the end of file) and a lot of assorted improvements. Not sure if I should open a separate bug for each one of them... I'll just attach the whole patchset here and if necessary will open separate bugs.
Created attachment 156150 [details] [review] Make index entries point at the beginning of a tag, not the end.
Created attachment 156151 [details] [review] Only put keyframes in the index
Created attachment 156152 [details] [review] Add a is-live property that disables index writing
Created attachment 156153 [details] [review] Seek to the beginning of output before writing the index
Created attachment 156154 [details] [review] Support streamheaders
Created attachment 156155 [details] [review] Mark buffers as delta units
Created attachment 156156 [details] [review] Make both sink pads accept either video or audio
Created attachment 156157 [details] [review] Make both sink pads accept either video or audio
Comment on attachment 156157 [details] [review] Make both sink pads accept either video or audio We can't do that because it would break the API of the element (different pad template names). What exactly is the problem you want to fix with this patch? gst_parse_launch() linking problems?
(In reply to comment #15) > (From update of attachment 156157 [details] [review]) > We can't do that because it would break the API of the element (different pad > template names). > > What exactly is the problem you want to fix with this patch? gst_parse_launch() > linking problems? My use case is that I'm receiving gdp encoded data from the network, so I first plug two fdsrc elements (followed by gdpdepay) into the muxer, and the decision which pad is audio and which is video is taken only after the connection is estabilished. So I'm trying to do something similar to gst-launch tcpclientsrc port=10000 ! gdpdepay ! queue ! flvmux name=muxer tcpclientsrc port=20000 ! gdpdepay ! queue ! muxer. muxer. ! fakesink and I get "already have a video pad" error from the muxer. I might be able to fix it by putting capsfilters after the gdp depayloaders, but I see that for instance oggmux has just one sink_%d request pad that accepts both audio and video.
(In reply to comment #16) > "already have a video pad" error from the muxer. I might be able to > fix it by putting capsfilters after the gdp depayloaders OK, I see now that I can (and should) fix it in application code, so forget about that one ;)
commit c69c5cb0d7445394037a2da9fd945dbe9abff596 Author: Jan Urbański <wulczer@wulczer.org> Date: Mon Mar 15 01:09:49 2010 +0100 flvmux: Correctly mark buffers as delta units Mark video interframes, video codec data buffers and audio buffers (if it's not an audio-only stream) as delta units. commit 9fdecbc1c11f4e5af6578bba32a9b32771029d33 Author: Jan Urbański <wulczer@wulczer.org> Date: Sun Mar 14 19:32:20 2010 +0100 flvmux: Support streamheaders Put the FLV header, the metadata tag and (if present) codec information in the streamheader to allow the muxer to be used for streaming. commit 7deee29d2ca48858ed6fb672246ef4be8c90d951 Author: Jan Urbański <wulczer@wulczer.org> Date: Sun Mar 14 01:38:21 2010 +0100 flvmux: Preallocate index space and fill it after finishing output Make the index appear at the beginning of the file, which is what most players are expecting. Fixes #601236. commit 7c74f7d52519cc5f5fcd65bfd91a47fc049bcdca Author: Sebastian Dröge <sebastian.droege@collabora.co.uk> Date: Mon Mar 15 13:47:13 2010 +0100 flvmux: Minor coding style fixes and cleanup commit 54a8237d625e9d84a4c25ae09a785af2d13f53ad Author: Jan Urbański <wulczer@wulczer.org> Date: Sun Mar 14 01:34:02 2010 +0100 flvmux: Add a is-live property If it is set, the muxer will not write the index. Defaults to false. commit c9bb3edd6fc6feb9dc30bc2183cf42cc3a062a72 Author: Jan Urbański <wulczer@wulczer.org> Date: Sun Mar 14 01:25:42 2010 +0100 flvmux: Only put valid seek points in the index For files containing video only video keyframes are valid points to which a player can seek. For audio-only files any tag start is a valid seek point. See #601236. commit b21c5c90152349f93a31938bca4464d4a703f308 Author: Jan Urbański <wulczer@wulczer.org> Date: Sun Mar 14 01:09:37 2010 +0100 flvmux: Fix index building to make entries point to tag's start offset Previous coding was wrongly incrementing the total byte count before adding an index entry.