[Home]

Summary:ASTERISK-08556: Incorrect parsing of IAX2 video frames
Reporter:mihai (mihai)Labels:
Date Opened:2007-01-11 09:13:21.000-0600Date Closed:2007-01-29 18:17:07.000-0600
Priority:MinorRegression?No
Status:Closed/CompleteComponents:Core/General
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:
Description:The header of IAX2 video frames is not parsed correctly in chan_iax2.c,  socket_process().  The current code looks like this (at or around line 7504):

fr->ts = (iaxs[fr->callno]->last & 0xFFFF8000L) | (ntohs(mh->ts) & 0x7fff);

This uses a mini-frame header struct to parse a video frame, which yields incorrect results.  Correct code should look like this:

fr->ts = (iaxs[fr->callno]->last & 0xFFFF0000L) | ntohs(vh->ts);

A similar problem can be found three lines above, in the IAXTESTS block.

The bug manifests itself when doing a video call with jitterbuffer enabled.  The code extracts video timestampts from the wrong place (the source id), which means the timestamp will be constant for the duration of the call.  This in turn confuses the jitterbuffer and results in frozen video followed by bursts of high speed video activity.

****** ADDITIONAL INFORMATION ******

I marked this as a 1.4.0 bug, but the problem is also present in trunk and 1.2.14.  Line numbers are correct for 1.4.0.
Comments:By: Serge Vecher (serge-v) 2007-01-11 09:21:53.000-0600

I'll mark this as 1.2.14, as this is where it needs to be fixed first. Would you be so kind as to get a disclaimer on file and submit the change as a patch?
By: mihai (mihai) 2007-01-11 09:46:07.000-0600

I can certainly do that, but I will have to check with with my employer before submitting a disclaimer, since I did this as part of my employment.  I don't expect any problems though, our company has contributed to Asterisk before.

I already have a patch against the 1.4.0 tarball that I can submit as soon as the legalities work out. Do you also need a patch against 1.2.14? I did not check whether my patch applies to 1.2.14 or trunk, but I expect that it would.
By: Serge Vecher (serge-v) 2007-01-11 09:49:45.000-0600

>Do you also need a patch against 1.2.14?
Yes, please.
By: Joshua C. Colp (jcolp) 2007-01-11 11:24:29.000-0600

This is such a minor change that I would say a disclaimer would be appreciated but not absolutely required.
By: mihai (mihai) 2007-01-12 08:57:50.000-0600

I agree, my initial intention was to provide sufficient information so that anybody to go in and fix this - in order to avoid the entire disclaimer headache.  I will, however fax in a disclaimer sometime today, it might be usefull for future contibutions.

That being said, I think there's a little confusion regarding timestamps in video frames.  The latest IAX2 draft claims the timestamp is a 16 bit field.  The bitfield diagram however, shows the MSB as '?'.  The current implementations mask out that bit, so I believe that means the timestamp is de facto a 15 bit field. This means my correction should be

fr->ts = (iaxs[fr->callno]->last & 0xFFFF8000L) | (ntohs(vh->ts) & 0x7fff);

Can anybody shed some light on this issue?
By: Russell Bryant (russell) 2007-01-29 18:17:06.000-0600

This has been fixed in 1.2, 1.4, and trunk in revisions 52762, 52763 and 52764.  Thanks!