[FFmpeg-devel] [PATCH 1/5] vorbis: handle special packets in the middle of a stream

[Michael Niedermayer]

On Sat, Oct 19, 2013 at 09:40:42AM -0400, Ben Boeckel wrote:
> This allows for updating metadata from new metadata packets in the
> middle of a stream (e.g., MPD streams). There still needs to be a signal
> that there *is* new metadata, but this is at least gets the data into a
> data structure.
> ---
>  libavcodec/vorbis_parser.c   | 20 ++++++++++++++++++--
>  libavcodec/vorbis_parser.h   |  6 +++++-
>  libavformat/oggparsevorbis.c | 17 +++++++++++++----
>  3 files changed, 36 insertions(+), 7 deletions(-)
> 
> diff --git a/libavcodec/vorbis_parser.c b/libavcodec/vorbis_parser.c
> index fcbecc8..d3d7ab7 100644
> --- a/libavcodec/vorbis_parser.c
> +++ b/libavcodec/vorbis_parser.c
> @@ -202,7 +202,7 @@ int avpriv_vorbis_parse_extradata(AVCodecContext *avctx, VorbisParseContext *s)
>  }
>  
>  int avpriv_vorbis_parse_frame(VorbisParseContext *s, const uint8_t *buf,
> -                              int buf_size)
> +                              int buf_size, int *flags)
>  {

breaks ABI between libs
you can neither remove nor change a function that is used from outside
the lib that you change. This affects all functions with name starting
with "av"



>      int duration = 0;
>  
> @@ -211,6 +211,22 @@ int avpriv_vorbis_parse_frame(VorbisParseContext *s, const uint8_t *buf,
>          int previous_blocksize = s->previous_blocksize;
>  
>          if (buf[0] & 1) {
> +            /* If the user doesn't care about special packets, it's a bad one. */
> +            if (!flags)
> +                goto bad_packet;
> +
> +            /* Set the flag for which kind of special packet it is. */
> +            if (buf[0] == 1)
> +                *flags |= VORBIS_FLAG_HEADER;
> +            else if (buf[0] == 3)
> +                *flags |= VORBIS_FLAG_COMMENT;
> +            else
> +                goto bad_packet;
> +
> +            /* Special packets have no duration. */
> +            return 0;
> +
> +bad_packet:




>              av_log(s->avctx, AV_LOG_ERROR, "Invalid packet\n");
>              return AVERROR_INVALIDDATA;
>          }
> @@ -252,7 +268,7 @@ static int vorbis_parse(AVCodecParserContext *s1, AVCodecContext *avctx,
>          if (avpriv_vorbis_parse_extradata(avctx, s))
>              goto end;
>  
> -    if ((duration = avpriv_vorbis_parse_frame(s, buf, buf_size)) >= 0)
> +    if ((duration = avpriv_vorbis_parse_frame(s, buf, buf_size, NULL)) >= 0)
>          s1->duration = duration;
>  
>  end:
> diff --git a/libavcodec/vorbis_parser.h b/libavcodec/vorbis_parser.h
> index 101df5d..54b780b 100644
> --- a/libavcodec/vorbis_parser.h
> +++ b/libavcodec/vorbis_parser.h
> @@ -50,6 +50,9 @@ typedef struct VorbisParseContext {
>   */
>  int avpriv_vorbis_parse_extradata(AVCodecContext *avctx, VorbisParseContext *s);
>  
> +#define VORBIS_FLAG_HEADER  0x00000001
> +#define VORBIS_FLAG_COMMENT 0x00000002
> +
>  /**
>   * Get the duration for a Vorbis packet.
>   *
> @@ -59,9 +62,10 @@ int avpriv_vorbis_parse_extradata(AVCodecContext *avctx, VorbisParseContext *s);
>   * @param s        Vorbis parser context
>   * @param buf      buffer containing a Vorbis frame
>   * @param buf_size size of the buffer
> + * @param flags    flags for special frames (NULL for "don't care")
>   */
>  int avpriv_vorbis_parse_frame(VorbisParseContext *s, const uint8_t *buf,
> -                              int buf_size);
> +                              int buf_size, int *flags);
>  
>  void avpriv_vorbis_parse_reset(VorbisParseContext *s);
>  
> diff --git a/libavformat/oggparsevorbis.c b/libavformat/oggparsevorbis.c
> index 36ad738..cca6de7 100644
> --- a/libavformat/oggparsevorbis.c
> +++ b/libavformat/oggparsevorbis.c
> @@ -345,7 +345,7 @@ static int vorbis_packet(AVFormatContext *s, int idx)
>      struct ogg *ogg = s->priv_data;
>      struct ogg_stream *os = ogg->streams + idx;
>      struct oggvorbis_private *priv = os->private;
> -    int duration;
> +    int duration, flags = 0;
>  
>      /* first packet handling
>       * here we parse the duration of each packet in the first page and compare

> @@ -359,19 +359,25 @@ static int vorbis_packet(AVFormatContext *s, int idx)
>          avpriv_vorbis_parse_reset(&priv->vp);
>          duration = 0;
>          seg = os->segp;
> -        d = avpriv_vorbis_parse_frame(&priv->vp, last_pkt, 1);
> +        d = avpriv_vorbis_parse_frame(&priv->vp, last_pkt, 1, &flags);
>          if (d < 0) {
>              os->pflags |= AV_PKT_FLAG_CORRUPT;
>              return 0;
> +        } else if (flags) {
> +            vorbis_header(s, idx);
> +            flags = 0;
>          }

this looks quite wrong
vorbis_header() can change codec values like channels and extradata
these values can be set initially by the demuxer but not changed
afterwards in that way as the decoder can be run in a seperate thread
and using them and there can be a packet que between demuxer and
decoder.
Also the demuxer already passes these packets to the decoder which
should update extradata accordingly

I see some subsequent patch from you replaces the vorbis_header()
calls again. I dont know if that solves the problem or not as its
hard to follow what gets replaced and whats there then.
Possibly you could split the changes in a more logic way so theres
no code added that just gets removed or replaced later
also each patch should generally only touch one lib, this reduces
the chances of breaking ABI


[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

No human being will ever know the Truth, for even if they happen to say it
by chance, they would not even known they had done so. -- Xenophanes
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20131019/899107ac/attachment.asc>