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