Hi, This cleans up FMTP parsing code somewhat. I only did it for H.264 and MPEG-4/AAC; if this approach is OK, I'll do the rest (Xiph, AMR, etc). 0001 -- rtpdec.h is a better place for SPACE_CHARS. Removes a dependency on internal.h, and avoids having to #include it in later patches. 0002 -- self explanatory. It is likely I will have to change value[4096] to be malloc'd due to massive Xiph extradata. 003-005 -- H.264 related 0006-007 -- MPEG-4/AAC related Josh
On 06/26/2010 12:28 AM, Josh Allmann wrote:
Hi,
This cleans up FMTP parsing code somewhat. I only did it for H.264 and MPEG-4/AAC; if this approach is OK, I'll do the rest (Xiph, AMR, etc).
0001 -- rtpdec.h is a better place for SPACE_CHARS. Removes a dependency on internal.h, and avoids having to #include it in later patches.
Fine
0002 -- self explanatory. It is likely I will have to change value[4096] to be malloc'd due to massive Xiph extradata.
+int ff_parse_fmtp(AVFormatContext *s, int st_index, + void *data, const char *p, + int (*parse_fmtp)(AVFormatContext *s, int st_index, + PayloadContext *data, + char *attr, char *value)) Why void data? why AVFormatContext *s, int st_index ? When passing the stream isn't enough? + while (!(res = strspn(p, SPACE_CHARS))) p++; // protocol identifier are you sure you need that?
003-005 -- H.264 related
0003 Ok 0004 - sdp_parse_fmtp_config_h264(stream, h264_data, attr, value); - } + return ff_parse_fmtp(s, st_index, h264_data, p, + &sdp_parse_fmtp_config_h264); sdp_parse_fmtp_config_h264 takes a stream as argument ff_parse_fmtp parse_fmtp doesn't seem to match the function. 0005 Ok
0006-007 -- MPEG-4/AAC related
both Ok Please check again 0004 =) lu -- Luca Barbato Gentoo/linux http://dev.gentoo.org/~lu_zero
Hi, On 25 June 2010 16:10, Luca Barbato <lu_zero@gentoo.org> wrote:
On 06/26/2010 12:28 AM, Josh Allmann wrote:
Hi,
This cleans up FMTP parsing code somewhat. I only did it for H.264 and MPEG-4/AAC; if this approach is OK, I'll do the rest (Xiph, AMR, etc).
0002 -- self explanatory. It is likely I will have to change value[4096] to be malloc'd due to massive Xiph extradata.
+int ff_parse_fmtp(AVFormatContext *s, int st_index, + void *data, const char *p, + int (*parse_fmtp)(AVFormatContext *s, int st_index, + PayloadContext *data, + char *attr, char *value))
Why void data? why AVFormatContext *s, int st_index ? When passing the stream isn't enough?
Fixed void data. I was trying to play it safe with passing the format context, but seems none of the depacketizers need it, so changed to AVStream.
+ while (!(res = strspn(p, SPACE_CHARS))) p++; // protocol identifier
are you sure you need that?
Reverted to the old way of doing things: + while (*p && *p != ' ') p++; // eat protocol identifier
003-005 -- H.264 related
0003 Ok
0004
- sdp_parse_fmtp_config_h264(stream, h264_data, attr, value); - } + return ff_parse_fmtp(s, st_index, h264_data, p, + &sdp_parse_fmtp_config_h264);
sdp_parse_fmtp_config_h264 takes a stream as argument ff_parse_fmtp parse_fmtp doesn't seem to match the function.
Not sure what you mean (I changed the function signature for sdp_parse_fmtp_config in the other hunk), but that's moot since ff_parse_fmtp takes a stream now.
On Fri, 25 Jun 2010, Josh Allmann wrote:
Not sure what you mean (I changed the function signature for sdp_parse_fmtp_config in the other hunk), but that's moot since ff_parse_fmtp takes a stream now.
All of these look good to me. I guess you checked that the parameters that you pass to the parser and parser callback suit the other ones (xiph, amr) too, so you won't have to change the interface when fixing the rest? // Martin
On 27 June 2010 10:25, Martin Storsjö <martin@martin.st> wrote:
All of these look good to me. I guess you checked that the parameters that you pass to the parser and parser callback suit the other ones (xiph, amr) too, so you won't have to change the interface when fixing the rest?
Yes, the only required changes are internal. Those are in patch 0008, which I can squash with 0002 if you'd prefer. Xiph and AMR cleanups attached, but I'd appreciate it if someone could test the AMR and Vorbis audio stuff for me. Josh
On Mon, 28 Jun 2010, Josh Allmann wrote:
On 27 June 2010 10:25, Martin Storsjö <martin@martin.st> wrote:
All of these look good to me. I guess you checked that the parameters that you pass to the parser and parser callback suit the other ones (xiph, amr) too, so you won't have to change the interface when fixing the rest?
Yes, the only required changes are internal. Those are in patch 0008, which I can squash with 0002 if you'd prefer.
I'm ok with it either way.
Xiph and AMR cleanups attached, but I'd appreciate it if someone could test the AMR and Vorbis audio stuff for me.
The AMR cleanup isn't totally right. Btw - the sample_50kbit.3gp sample in DSS has AMR, you can test with that. I have no Vorbis testing setup at the moment, but isn't that quite easy to set up with feng?
-static int amr_parse_sdp_line(AVFormatContext *s, int st_index, - PayloadContext *data, const char *line) +static int amr_parse_fmtp(AVStream *stream, PayloadContext *data, + char *attr, char *value) { - const char *p; - char attr[25], value[25]; - - /* Parse an fmtp line this one: - * a=fmtp:97 octet-align=1; interleaving=0 - * That is, a normal fmtp: line followed by semicolon & space - * separated key/value pairs. - */ - if (av_strstart(line, "fmtp:", &p)) { int octet_align = 0; int crc = 0; int interleaving = 0; int channels = 1;
[cut]
@@ -159,11 +145,26 @@ static int amr_parse_sdp_line(AVFormatContext *s, int st_index, interleaving = atoi(value); else if (!strcmp(attr, "channels")) channels = atoi(value); - } if (!octet_align || crc || interleaving || channels != 1) { - av_log(s, AV_LOG_ERROR, "Unsupported RTP/AMR configuration!\n"); + av_log(stream, AV_LOG_ERROR, "Unsupported RTP/AMR configuration!\n"); return -1; }
These variables, octet_align, crc, interleaving, channels, were local to the amr_parse_sdp_line function earlier - now they're local to amr_parse_fmtp. This means that they're reset to the default values after each invocation of amr_parse_fmtp, and we're unable to verify that the complete fmtp line had a valid configuration. In practice, this would trigger a warning after parsing any other configuration item than octet-align, since octet_align would be left at 0 when parsing those other configuration items. The sample only has octet-align and no other configuration items, though, so you wouldn't hit this issue with that. So those variables should be moved to the payload context in this case - even if they're not used by the packet parsing routine (yet at least).
+ if (av_strstart(line, "fmtp:", &p)) { + return ff_parse_fmtp(s->streams[st_index], data, p, + &amr_parse_fmtp); }
Btw, the common coding style is to omit & for function pointers, I think. // Martin
On Sun, 27 Jun 2010, Martin Storsjö wrote:
On Fri, 25 Jun 2010, Josh Allmann wrote:
Not sure what you mean (I changed the function signature for sdp_parse_fmtp_config in the other hunk), but that's moot since ff_parse_fmtp takes a stream now.
All of these look good to me.
Applied all parts from this bunch, with a few minor cosmetic changes. (Removed & from calls where function pointers are given as parameters, removed extra braces from the otherwise pure reindent cosmetic patch.) When rebasing the rest on top of this, you'll probably need to do rebase --skip when it complains about conflicts, for those hunks that were slightly modified. // Martin
On 28 June 2010 01:51, Martin Storsjö <martin@martin.st> wrote:
On Mon, 28 Jun 2010, Josh Allmann wrote:
Xiph and AMR cleanups attached, but I'd appreciate it if someone could test the AMR and Vorbis audio stuff for me.
The AMR cleanup isn't totally right. Btw - the sample_50kbit.3gp sample in DSS has AMR, you can test with that.
Seems OK.
I have no Vorbis testing setup at the moment, but isn't that quite easy to set up with feng?
Before/after CRCs are the same, so I suppose it works.
These variables, octet_align, crc, interleaving, channels, were local to the amr_parse_sdp_line function earlier - now they're local to amr_parse_fmtp. This means that they're reset to the default values after each invocation of amr_parse_fmtp, and we're unable to verify that the complete fmtp line had a valid configuration.
In practice, this would trigger a warning after parsing any other configuration item than octet-align, since octet_align would be left at 0 when parsing those other configuration items. The sample only has octet-align and no other configuration items, though, so you wouldn't hit this issue with that.
So those variables should be moved to the payload context in this case - even if they're not used by the packet parsing routine (yet at least).
10l to me. Fixed.
+ if (av_strstart(line, "fmtp:", &p)) { + return ff_parse_fmtp(s->streams[st_index], data, p, + &amr_parse_fmtp); }
Btw, the common coding style is to omit & for function pointers, I think.
Corrected in the Xiph and AMR patches. Josh
On Mon, 28 Jun 2010, Josh Allmann wrote:
Corrected in the Xiph and AMR patches.
Applied with a few minor changes: - Allocated an extra byte for the null terminator in the value buffer in ff_parse_fmtp. (On second thought, though, it shouldn't ever be needed, so this point is moot) - Moved the checking of the AMR fmtp parameters to after parsing the whole line, instead of checking again after each item. - You can't use an AVStream as av_log context, it doesn't have an AVClass // Martin
participants (3)
-
Josh Allmann -
Luca Barbato -
Martin Storsjö