[FFmpeg-soc] [PATCH] fmtp cleanup

[Martin Storsjö]

On Mon, 28 Jun 2010, Josh Allmann wrote:

> On 27 June 2010 10:25, Martin Storsjö <martin at 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