GNOME Bugzilla – Bug 509311
[rtph263pay] rtph263pay does not follow rfc2190
Last modified: 2009-04-14 16:56:07 UTC
Please describe the problem: The current implementation is just a hack that applies payload and rtp headers to the actual data without MTU and rfc2190 consideration. The output packages are bigger than MTU. The frames should be payloaded at least in payload mode A and/or B that are specified in the mentioned rfc2190. Steps to reproduce: 1. gst-launch videotestsrc pattern=0 num-buffers=3 ! ffenc_h263 rtp-payload-size=1 ! rtph263pay ! fakesink dump=true Actual results: you get a dump of the buffers that are actually bigger than MTU Expected results: to get buffers <= MTU and properly payloaded Does this happen every time? yes Other information:
Created attachment 102796 [details] [review] Patch fixing mode A and B Not final, lot of issues still open
Created attachment 102797 [details] [review] Fixed patch Not to be accepted yet
Created attachment 102921 [details] [review] Newer patch - Some issues fixed - refactoring
Created attachment 103063 [details] [review] Major refactoring and debugging - still not to commit - payload mode B needs some more fixing
Created attachment 103146 [details] [review] Newer patch - Added "modea-only" property - default=TRUE, for now - Fixed some issues - Some debugging on mode B
Created attachment 103458 [details] [review] Pre-alpha patch - mode B mostly "fixed" - needs a lot of testing - needs MVD and some other data in B packets yet - debug statements remove-al Please test it.
Created attachment 104177 [details] [review] Newer rewritten, unstable patch - a nicer implementation, maybe needs some memory optimization - mode A working - mode B still not working
Created attachment 104205 [details] [review] Bug fixes to the previous patch - Fixes in mode A - Fixes in mode B - still some decoding problems - Fixed some memory leaks Please test it
Created attachment 104398 [details] [review] Patch implementing rfc - Bug fixes - Mode B has still problems
Created attachment 104629 [details] [review] Patch with some fixes in mode B - some fixes in mode B
Created attachment 104950 [details] [review] Minor changes to previous patch
There's a problem in mode B i can't detect, i will be pleased if anyone with a bit of time could take a look at this stuff. When finding macro-block boundaries for mode B payloading, there comes an error due to a code that is not found in the code tables. I guess this is due to a wrong decoding process due to misinterpretation of: http://www.itu.int/rec/dologin_pub.asp?lang=e&id=T-REC-H.263-199603-S!!PDF-E&type=items For debugging i use the following line: time GST_DEBUG_NO_COLOR=1 GST_DEBUG=rtph263pay:5 gst-launch videotestsrc pattern=0 num-buffers=100 ! ffenc_h263 rtp-payload-size=1 ! rtph263pay mtu=1400 modea-only=false ! rtph263depay ! ffdec_h263 ! sdlvideosink &> log When a decode error accurs, the code skips the current GOB and emits a WARNING. Thanks
Created attachment 105338 [details] [review] Some fixes - there are some problems in B mode packetization - there is still a problem with decoding
Created attachment 105990 [details] [review] Fixed intra frame decoding - spooky first (intra) frame - but no decode errors - still decoding problems with inter frames PLEASE TEST with modea-only=false
Created attachment 106139 [details] [review] Latest pre-rc patch
Is this patch now committable?
No. The patch is not commitable yet. - It crashes if ffenc_h263 has the property "rtp-payload-size" set to 0. - for this, i guess a fix won't be so difficult - The mode B payloader procedure has still an issue with some P frames. - it fails on certain GOBs that seem to have a strange structure. I couldn't find the reason why. What actually happens is that at some point a value is badly decoded and the bit-shift shifts to an unknown code. Maybe i misunderstood the specification. And that's why i can't find the reason. - this involves debugging and decoding by hand, which takes a lot of time - if someone could possibly take a look...
I have just test your patch. The parser miss the first GOB of frame because in function gst_rtp_h263_pay_gobfinder we always try to find GBSC marker, but for the first GOB there is not header (itu spec §5.2). I think that introduice a bad bit-shift.
I never search for the header of the first GOB. I search for the header of the 2+nd GOB. The GOB finder function works, because mode A works as it should. If you'll look closely to the implementation, it actually starts at the 4th byte of the frame, so it searches GBSCs that are of higher GOBN than 1. The problem is in mode B. There's the bit shift problem. So inside a GOB (MB layer) not outside. Actually the problem is somewhere in the _mb_finder() function.
while parsing macro block we only take care of the type of the macro block to get the number of MVD to parse, but in itu spec " 5.3.8 Motion Vector Data (MVD 2-4) are included if indicated by PTYPE and by MCBPC, and consist each of a variable length The three codewords MVD 2-4 codeword for the horizontal component followed by a variable length codeword for the vertical component of each are only present when in Advanced Prediction mode (refer vector. Variable length codes are given in Table 11. MVD 2-4 to Annex F). " So maybe we need to check if the Advanced Prediction mode is 1 or 0 to get the number of MVD ?
Created attachment 129846 [details] [review] pre-rc bugfix. This patch fixes invalid parsing of a GOB header. It is still not commitable because mode C from rfc2190 is not yet implemented.
Created attachment 130397 [details] [review] New patch - RC So the previous patch didn't really fix anything... But this one does! Both bugs should be fixed.
Created attachment 130509 [details] [review] new payloader - RC2 New patch removec amode-only property and still works event if rtp-payload-size is set to 0.
I think this patch is in the final state. Please check/test it.
Created attachment 132297 [details] [review] patch over latest patch Hi, First, thanks for your awesome work! Here are my comments about the latest patch : 1 - mode A is broken because there are a few #if 0 in there to remove mode A support.. I know you're currently testing mode B, but it doesn't have to break mode A... 2 - Choosing the mode is important because I'm trying to make this work with Window's Live Messenger compatibility and they only support mode A.. but when I try with your code, it forces the use of mode B and I don't know exactly how it decides which mode to use, I would want to force it to always use mode A. 3 - there's a memory leak in gst_rtp_h263_pay_gobfinder, you free the GstRtpH263PayBoundry when you can't find any GOB, but you don't free it when you actually find the GOB. 4 - the marker was always set to TRUE because you commented out the lines that check wheter it should be set to TRUE or not (probably for debugging and forgot to revert) 5 - If the rtp-payload-size property on ffenc_h263 isn't set to "1", then the payloader won't work, because it loops trying to find each GOB one by one, and if it doesn't find one, it will give the warning : "No GOB's were found in data stream! Please enable RTP mode in encoder. Forcing mode A for now." and it will send the entire frame... the problem is that if you set the rtp-payload-size to 1024 for example, you may have only 2 or 3 gob headers in your stream and the payloader should split the packet on those gobs, and not try to find each gob individually then add them one by one and see if the size is < mtu. Besides, the warning says that it couldn't find any GOBs, while it actually found quite a few, but it didn't find them *all*. 6 - I don't think that the payloader should require the encoder to add the GOB headers since these are completely optional. What it should do is to actually scan the stream and find the GOBs without the header (I think you already do that for the B mode), and use that to split the message using the sbit and ebit header fields. I say this because WLM doesn't use GOB headers, it just splits it at GOB boundary and uses the sbit and ebit fields. If the code is already there for mode B, then I think it should be used also for mode A and find the gob boundaries correctly without relying on the GOB headers being there. I'm attaching a patch to the previous patch (so apply the latest patch then apply mine). I did it that way so you can see what I changed from your code. Thanks again for your work! KaKaRoTo
commit 17d9cb3319d93a9a5ab8833c26bb1bc7326aeb63 Author: Youness Alaoui <youness.alaoui at collabora.co.uk> Date: Tue Apr 14 18:52:48 2009 +0200 h263pay: various fixes Re-enable mode A support and a property to control it. Fix memory leak of GstRtpH263PayBoundry objects. Fix marker. Fixes #509311 commit de2c4895268948e10f37be29376c10ee64965a52 Author: Janin Kolenc <janin.kolenc at marand.si> Date: Tue Apr 14 18:44:51 2009 +0200 h263pay: Fix the payloader Fix the H263 payloader to be more RFC 2190 compliant. See #509311