[FFmpeg-devel] [PATCH] lavc: add ff_bprint_to_extradata() helper and use it.
Nicolas George
nicolas.george at normalesup.org
Sun Dec 30 14:52:08 CET 2012
Le nonidi 9 nivôse, an CCXXI, Clement Boesch a écrit :
> At the same time, it should fix 'warning: dereferencing type-punned
> pointer will break strict-aliasing rules' warning for compilers who
> don't consider uint8_t** and char** compatibles.
> ---
> libavcodec/dvdsubenc.c | 5 +++--
> libavcodec/internal.h | 6 ++++++
> libavcodec/utils.c | 14 ++++++++++++++
> libavformat/assdec.c | 8 +++-----
> libavformat/jacosubdec.c | 8 +++++---
> libavformat/samidec.c | 9 +++------
> libavformat/subviewerdec.c | 8 +++-----
> 7 files changed, 37 insertions(+), 21 deletions(-)
>
> diff --git a/libavcodec/dvdsubenc.c b/libavcodec/dvdsubenc.c
> index 2e0c37d..352272d 100644
> --- a/libavcodec/dvdsubenc.c
> +++ b/libavcodec/dvdsubenc.c
> @@ -20,6 +20,7 @@
> */
> #include "avcodec.h"
> #include "bytestream.h"
> +#include "internal.h"
> #include "libavutil/avassert.h"
> #include "libavutil/bprint.h"
> #include "libavutil/imgutils.h"
> @@ -408,9 +409,9 @@ static int dvdsub_init(AVCodecContext *avctx)
> av_bprintf(&extradata, " %06"PRIx32"%c",
> dvdc->global_palette[i] & 0xFFFFFF, i < 15 ? ',' : '\n');
>
> - if ((ret = av_bprint_finalize(&extradata, (char **)&avctx->extradata)) < 0)
> + ret = ff_bprint_to_extradata(avctx, &extradata);
> + if (ret < 0)
> return ret;
> - avctx->extradata_size = extradata.len;
>
> return 0;
> }
> diff --git a/libavcodec/internal.h b/libavcodec/internal.h
> index 2d3433f..69210a3 100644
> --- a/libavcodec/internal.h
> +++ b/libavcodec/internal.h
> @@ -26,6 +26,7 @@
>
> #include <stdint.h>
>
> +#include "libavutil/bprint.h"
> #include "libavutil/mathematics.h"
> #include "libavutil/pixfmt.h"
> #include "avcodec.h"
> @@ -201,4 +202,9 @@ int ff_codec_open2_recursive(AVCodecContext *avctx, const AVCodec *codec, AVDict
> */
> int ff_codec_close_recursive(AVCodecContext *avctx);
>
> +/**
> + * Finalize buf into extradata and set its size appropriately.
> + */
> +int ff_bprint_to_extradata(AVCodecContext *avctx, AVBPrint *buf);
For this kind of situation, I believe it is better to use the structure
name:
int ff_bprint_to_extradata(AVCodecContext *avctx, struct AVBPrint *buf);
It reduces the inter-header dependencies and recompilation hell.
> +
> #endif /* AVCODEC_INTERNAL_H */
> diff --git a/libavcodec/utils.c b/libavcodec/utils.c
> index b366995..7c7e502 100644
> --- a/libavcodec/utils.c
> +++ b/libavcodec/utils.c
> @@ -2684,3 +2684,17 @@ int avcodec_is_open(AVCodecContext *s)
> {
> return !!s->internal;
> }
> +
> +int ff_bprint_to_extradata(AVCodecContext *avctx, AVBPrint *buf)
> +{
> + int ret;
> + char *str;
> +
> + ret = av_bprint_finalize(buf, &str);
> + if (ret < 0)
> + return ret;
> + avctx->extradata = str;
avctx->extradata is supposed to be padded to FF_INPUT_BUFFER_PADDING_SIZE,
IIRC.
> + /* extradata needs to contain '\0' in case it is copied around */
> + avctx->extradata_size = buf->len + 1;
For the codecs I know (at least ASS- and dvdsub-in-Matroska), the
terminating NUL is not part of the extradata, and therefore the +1 is wrong.
If you know codecs where the terminating NUL should be included, a flag is
probably the answer.
> + return 0;
> +}
> diff --git a/libavformat/assdec.c b/libavformat/assdec.c
> index 6c4614e..34229cb 100644
> --- a/libavformat/assdec.c
> +++ b/libavformat/assdec.c
> @@ -22,6 +22,7 @@
> #include "avformat.h"
> #include "internal.h"
> #include "subtitles.h"
> +#include "libavcodec/internal.h"
> #include "libavutil/bprint.h"
>
> typedef struct ASSContext{
> @@ -132,12 +133,9 @@ static int ass_read_header(AVFormatContext *s)
>
> av_bprint_finalize(&line, NULL);
>
> - av_bprint_finalize(&header, (char **)&st->codec->extradata);
> - if (!st->codec->extradata) {
> - res = AVERROR(ENOMEM);
> + res = ff_bprint_to_extradata(st->codec, &header);
> + if (res < 0)
> goto end;
> - }
> - st->codec->extradata_size = header.len + 1;
The "+1" was wrong.
(Strangely, I can not see its effect in the resulting MKV file; the Matroska
muxer is probably doing some magic too.)
>
> ff_subtitles_queue_finalize(&ass->q);
>
> diff --git a/libavformat/jacosubdec.c b/libavformat/jacosubdec.c
> index 1c58e3b..153da42 100644
> --- a/libavformat/jacosubdec.c
> +++ b/libavformat/jacosubdec.c
> @@ -28,6 +28,7 @@
> #include "avformat.h"
> #include "internal.h"
> #include "subtitles.h"
> +#include "libavcodec/internal.h"
> #include "libavcodec/jacosub.h"
> #include "libavutil/avstring.h"
> #include "libavutil/bprint.h"
> @@ -159,7 +160,7 @@ static int jacosub_read_header(AVFormatContext *s)
> JACOsubContext *jacosub = s->priv_data;
> int shift_set = 0; // only the first shift matters
> int merge_line = 0;
> - int i;
> + int i, ret;
>
> AVStream *st = avformat_new_stream(s, NULL);
> if (!st)
> @@ -228,8 +229,9 @@ static int jacosub_read_header(AVFormatContext *s)
> }
>
> /* general/essential directives in the extradata */
> - av_bprint_finalize(&header, (char **)&st->codec->extradata);
> - st->codec->extradata_size = header.len + 1;
Do you know any container that can have jacosub in it?
> + ret = ff_bprint_to_extradata(st->codec, &header);
> + if (ret < 0)
> + return ret;
>
> /* SHIFT and TIMERES affect the whole script so packet timing can only be
> * done in a second pass */
> diff --git a/libavformat/samidec.c b/libavformat/samidec.c
> index 85fd220..bdde2f4 100644
> --- a/libavformat/samidec.c
> +++ b/libavformat/samidec.c
> @@ -27,6 +27,7 @@
> #include "avformat.h"
> #include "internal.h"
> #include "subtitles.h"
> +#include "libavcodec/internal.h"
> #include "libavutil/avstring.h"
> #include "libavutil/bprint.h"
> #include "libavutil/intreadwrite.h"
> @@ -91,13 +92,9 @@ static int sami_read_header(AVFormatContext *s)
> av_bprint_clear(&buf);
> }
>
> - st->codec->extradata_size = hdr_buf.len + 1;
Same question for SAMI?
> - av_bprint_finalize(&hdr_buf, (char **)&st->codec->extradata);
> - if (!st->codec->extradata) {
> - st->codec->extradata_size = 0;
> - res = AVERROR(ENOMEM);
> + res = ff_bprint_to_extradata(st->codec, &hdr_buf);
> + if (res < 0)
> goto end;
> - }
>
> ff_subtitles_queue_finalize(&sami->q);
>
> diff --git a/libavformat/subviewerdec.c b/libavformat/subviewerdec.c
> index 7691d82..8ecc928 100644
> --- a/libavformat/subviewerdec.c
> +++ b/libavformat/subviewerdec.c
> @@ -27,6 +27,7 @@
> #include "avformat.h"
> #include "internal.h"
> #include "subtitles.h"
> +#include "libavcodec/internal.h"
> #include "libavutil/avstring.h"
> #include "libavutil/bprint.h"
> #include "libavutil/intreadwrite.h"
> @@ -99,12 +100,9 @@ static int subviewer_read_header(AVFormatContext *s)
> av_bprintf(&header, "%s", line);
> if (!strncmp(line, "[END INFORMATION]", 17) || !strncmp(line, "[SUBTITLE]", 10)) {
> /* end of header */
> - av_bprint_finalize(&header, (char **)&st->codec->extradata);
> - if (!st->codec->extradata) {
> - res = AVERROR(ENOMEM);
> + res = ff_bprint_to_extradata(st->codec, &header);
> + if (res < 0)
> goto end;
> - }
> - st->codec->extradata_size = header.len + 1;
... and subviewer?
> } else if (strncmp(line, "[INFORMATION]", 13)) {
> /* assume file metadata at this point */
> int i, j = 0;
Regards,
--
Nicolas George
-------------- 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/20121230/8a6e256c/attachment.asc>
More information about the ffmpeg-devel
mailing list