[FFmpeg-devel] [PATCH] Allow mpjpeg demuxer to process MIME parts which do not include Content-Length header.
wm4
nfxjfg at googlemail.com
Tue Nov 24 16:32:52 CET 2015
On Tue, 24 Nov 2015 00:16:06 -0500
Alex Agranovsky <alex at sighthound.com> wrote:
> From a2a0b9e0da14b6e82aa783535ec1878c8ffbede0 Mon Sep 17 00:00:00 2001
> From: Alex Agranovsky <alex at sighthound.com>
> Date: Tue, 24 Nov 2015 00:06:14 -0500
> Subject: [PATCH 1/2] Allow mpjpeg demuxer to process MIME parts which do not
> include Content-Length header.
>
> Fixes ticket 5023
>
> Signed-off-by: Alex Agranovsky <alex at sighthound.com>
> ---
> libavformat/mpjpegdec.c | 176 ++++++++++++++++++++++++++++++++++++------------
> 1 file changed, 133 insertions(+), 43 deletions(-)
>
> diff --git a/libavformat/mpjpegdec.c b/libavformat/mpjpegdec.c
> index 2749a48..e494f1a 100644
> --- a/libavformat/mpjpegdec.c
> +++ b/libavformat/mpjpegdec.c
> @@ -23,22 +23,16 @@
>
> #include "avformat.h"
> #include "internal.h"
> +#include "avio_internal.h"
>
> -static int get_line(AVIOContext *pb, char *line, int line_size)
> -{
> - int i = ff_get_line(pb, line, line_size);
>
> - if (i > 1 && line[i - 2] == '\r')
> - line[i - 2] = '\0';
>
> - if (pb->error)
> - return pb->error;
> -
> - if (pb->eof_reached)
> - return AVERROR_EOF;
> -
> - return 0;
> -}
> +/** Context for demuxing an MPJPEG stream. */
This comment is really not needed.
> +typedef struct MPJPEGDemuxContext {
> + char* boundary;
> + char* searchstr;
> + int searchstr_len;
> +} MPJPEGDemuxContext;
>
>
> static void trim_right(char* p)
> @@ -46,13 +40,32 @@ static void trim_right(char* p)
> char *end;
> if (!p || !*p)
> return;
> - end = p + strlen(p) - 1;
> - while (end != p && av_isspace(*end)) {
> + int len = strlen(p);
> + if ( !len )
> + return;
> + end = p + len - 1;
> + while (end >= p && av_isspace(*end)) {
Why this change? Note that strlen(p)>0 is already guaranteed by the
"!*p" check before.
> *end = '\0';
> end--;
> }
> }
>
> +static int get_line(AVIOContext *pb, char *line, int line_size)
> +{
> + ff_get_line(pb, line, line_size);
> +
> + if (pb->error)
> + return pb->error;
> +
> + if (pb->eof_reached)
> + return AVERROR_EOF;
> +
> + trim_right(line);
> + return 0;
> +}
> +
> +
> +
> static int split_tag_value(char **tag, char **value, char *line)
> {
> char *p = line;
> @@ -86,12 +99,24 @@ static int split_tag_value(char **tag, char **value, char *line)
> return 0;
> }
>
> -static int parse_multipart_header(AVIOContext *pb, void *log_ctx);
> +static int parse_multipart_header(AVIOContext *pb,
> + int* size,
> + const char* expected_boundary,
> + void *log_ctx);
> +
> +static int mpjpeg_read_close(AVFormatContext *s)
> +{
> + MPJPEGDemuxContext *mpjpeg = s->priv_data;
> + av_freep(&mpjpeg->boundary);
> + av_freep(&mpjpeg->searchstr);
> + return 0;
> +}
>
> static int mpjpeg_read_probe(AVProbeData *p)
> {
> AVIOContext *pb;
> int ret = 0;
> + int size = 0;
>
> if (p->buf_size < 2 || p->buf[0] != '-' || p->buf[1] != '-')
> return 0;
> @@ -100,7 +125,7 @@ static int mpjpeg_read_probe(AVProbeData *p)
> if (!pb)
> return 0;
>
> - ret = (parse_multipart_header(pb, NULL)>0)?AVPROBE_SCORE_MAX:0;
> + ret = ( parse_multipart_header(pb, &size, "--", NULL) > 0 ) ? AVPROBE_SCORE_MAX : 0;
A stray space got in.
>
> av_free(pb);
>
> @@ -110,17 +135,19 @@ static int mpjpeg_read_probe(AVProbeData *p)
> static int mpjpeg_read_header(AVFormatContext *s)
> {
> AVStream *st;
> - char boundary[70 + 2 + 1];
> + char boundary[70 + 2 + 1] = {0};
> int64_t pos = avio_tell(s->pb);
> int ret;
>
> + do {
> + ret = get_line(s->pb, boundary, sizeof(boundary));
> + if (ret < 0)
> + return ret;
> + } while (!boundary[0]);
>
> - ret = get_line(s->pb, boundary, sizeof(boundary));
> - if (ret < 0)
> - return ret;
> -
Can you explain why it suddenly has to skip multiple lines?
> - if (strncmp(boundary, "--", 2))
> + if (strncmp(boundary, "--", 2)) {
> return AVERROR_INVALIDDATA;
> + }
Another change that looks like a stray change.
>
> st = avformat_new_stream(s, NULL);
> if (!st)
> @@ -147,28 +174,44 @@ static int parse_content_length(const char *value)
> return val;
> }
>
> -static int parse_multipart_header(AVIOContext *pb, void *log_ctx)
> +static int parse_multipart_header(AVIOContext *pb,
> + int* size,
> + const char* expected_boundary,
> + void *log_ctx)
> {
> char line[128];
> int found_content_type = 0;
> - int ret, size = -1;
> + int ret;
> +
> + *size = -1;
>
> // get the CRLF as empty string
> ret = get_line(pb, line, sizeof(line));
> - if (ret < 0)
> + if (ret < 0) {
> return ret;
> + }
Another stray change?
>
> /* some implementation do not provide the required
> * initial CRLF (see rfc1341 7.2.1)
> */
> - if (!line[0]) {
> + while (!line[0]) {
> ret = get_line(pb, line, sizeof(line));
> - if (ret < 0)
> + if (ret < 0) {
> return ret;
> + }
> }
Again, why does it have to skip multiple lines now? The comment
indicates that skipping 1 line (instead of nothing) is a workaround for
some buggy servers.
>
> - if (strncmp(line, "--", 2))
> + if ( strncmp(line, expected_boundary, strlen(expected_boundary) ) ) {
(Maybe could use av_strstart. Also, weird spacing.)
> + if (log_ctx) {
> + av_log(log_ctx,
> + AV_LOG_ERROR,
> + "Expected boundary '%s' not found, instead found a line of %lu bytes\n",
> + expected_boundary,
> + strlen(line) );
> + }
> +
> return AVERROR_INVALIDDATA;
> + }
>
> while (!pb->eof_reached) {
> char *tag, *value;
> @@ -201,32 +244,77 @@ static int parse_multipart_header(AVIOContext *pb, void *log_ctx)
> } else
> found_content_type = 1;
> } else if (!av_strcasecmp(tag, "Content-Length")) {
> - size = parse_content_length(value);
> - if (size < 0)
> - return size;
> + *size = parse_content_length(value);
> }
> }
>
> - if (!found_content_type || size < 0) {
> - return AVERROR_INVALIDDATA;
> - }
> -
> - return size;
> + return (found_content_type) ? 0 : AVERROR_INVALIDDATA;
> }
>
> +
> static int mpjpeg_read_packet(AVFormatContext *s, AVPacket *pkt)
> {
> - int ret;
> - int size = parse_multipart_header(s->pb, s);
> + int size;
> + MPJPEGDemuxContext *mpjpeg = s->priv_data;
> + if ( mpjpeg->boundary == NULL ) {
> + mpjpeg->boundary = av_strdup("--");
> + mpjpeg->searchstr = av_strdup("\r\n--");
> + mpjpeg->searchstr_len = strlen( mpjpeg->searchstr );
Missing checks for memory allocation failure.
Also, this is the only place where these fields are set. And they're
set to constant strings. I see no reason why these fields exist, and
they certainly don't need to be strdup'ed?
My understanding of this MIME part encoding is very limited, but I
thought you need to read the boundary as it appears in the header, and
use that to find the end of the data? Was this forgotten?
> + }
> +
> + int ret = parse_multipart_header(s->pb,
> + &size,
> + mpjpeg->boundary,
> + s);
Declaration after statements; apparently we don't like this, and it's
preferred to put all declarations on top of each block.
>
> - if (size < 0)
> - return size;
>
> - ret = av_get_packet(s->pb, pkt, size);
> if (ret < 0)
> return ret;
>
> - return 0;
> + if ( size > 0 ) {
> + /* size has been provided to us in MIME header */
> + ret = av_get_packet(s->pb, pkt, size);
> + } else {
> + /* no size was given -- we read until the next boundary or end-of-file */
> +
> + const int read_chunk = 2048;
> + av_init_packet(pkt);
> + pkt->data = NULL;
> + pkt->size = 0;
> + pkt->pos = avio_tell(s->pb);
> +
> + /* we may need to return as much as all we've read back to the buffer */
> + ffio_ensure_seekback( s->pb, read_chunk );
> +
> +
> + int remaining = 0, len;
> +
> + while ( ( ret = av_append_packet( s->pb, pkt, read_chunk - remaining ) ) >= 0 ) {
> + /* scan the new data */
> + len = ret + remaining;
> + char* start = pkt->data + pkt->size - len;
> + do {
> + if ( !memcmp( start, mpjpeg->searchstr, mpjpeg->searchstr_len ) ) {
How is it guaranteed that start is valid for at least searchstr_len
bytes?
> + // got the boundary! rewind the stream
> + avio_seek( s->pb, -(len-2), SEEK_CUR );
> + pkt->size -= (len-2);
> + return pkt->size;
> + }
> + len--;
> + start++;
> + } while ( len >= mpjpeg->searchstr_len );
Couldn't this use strstr? (Just a thought. Would assume that the
boundary itself ca not contain 0 bytes.)
> + remaining = len;
> + }
> +
> + /* error or EOF occurred */
> + if ( ret == AVERROR_EOF ) {
> + ret = ( pkt->size > 0 ) ? pkt->size : AVERROR_EOF;
Unneeded ( ).
> + } else {
> + av_packet_unref(pkt);
> + }
> + }
> +
> + return ret;
> }
The spacing in this whole function is a bit weird and inconsistent with
the rest of the code, I think.
>
> AVInputFormat ff_mpjpeg_demuxer = {
> @@ -234,7 +322,9 @@ AVInputFormat ff_mpjpeg_demuxer = {
> .long_name = NULL_IF_CONFIG_SMALL("MIME multipart JPEG"),
> .mime_type = "multipart/x-mixed-replace",
> .extensions = "mjpg",
> + .priv_data_size = sizeof(MPJPEGDemuxContext),
> .read_probe = mpjpeg_read_probe,
> .read_header = mpjpeg_read_header,
> .read_packet = mpjpeg_read_packet,
> + .read_close = mpjpeg_read_close
> };
More information about the ffmpeg-devel
mailing list