Re: [FFmpeg-devel] [PATCH] avcodec/h264_parse: Treat escaped and unescaped decoding error equal in decode_extradata_ps_mp4()
Would it be possible to cherry-pick into n3.4? Thanks! On 12/2/17 20:02, Michael Niedermayer wrote: On Sat, Nov 25, 2017 at 11:08:46PM +0100, Clément Bœsch wrote: On Sat, Nov 25, 2017 at 10:49:09PM +0100, Michael Niedermayer wrote: Fixes: lorex.mp4 Signed-off-by: Michael Niedermayer --- libavcodec/h264_parse.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/libavcodec/h264_parse.c b/libavcodec/h264_parse.c index a7c71d9bbb..9216d0bdbd 100644 --- a/libavcodec/h264_parse.c +++ b/libavcodec/h264_parse.c @@ -427,8 +427,6 @@ static int decode_extradata_ps_mp4(const uint8_t *buf, int buf_size, H264ParamSe ret = decode_extradata_ps(escaped_buf, escaped_buf_size, ps, 1, logctx); av_freep(&escaped_buf); -if (ret < 0) -return ret; If you don't want the return code to be reintroduced differently 10x in the future (like, someone deciding to return ret in the function instead of 0), I'd suggest 2 things: - use "(void)decode_extradata_ps(...)" to explicitly ignore the code return; it's a hint for the compiler and the developer, typically used in OpenBSD (I believe that's because they warn about unchecked return code by default) - add a comment above about the why will push with these changes thx [...] ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH] avfilter/vf_drawtext: fixed default/flt formatting ignoring offset parameter
From 9f183abbdde7fa50b9425165dd792d2770098749 Mon Sep 17 00:00:00 2001 From: Alex Agranovsky Date: Mon, 26 Sep 2016 16:23:45 -0400 Subject: [PATCH] avfilter/vf_drawtext: fixed default/flt formatting ignoring offset parameter Signed-off-by: Alex Agranovsky --- libavfilter/vf_drawtext.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libavfilter/vf_drawtext.c b/libavfilter/vf_drawtext.c index 214aef0..489db40 100644 --- a/libavfilter/vf_drawtext.c +++ b/libavfilter/vf_drawtext.c @@ -809,7 +809,7 @@ static int func_pts(AVFilterContext *ctx, AVBPrint *bp, pts += (double)delta / AV_TIME_BASE; } if (!strcmp(fmt, "flt")) { -av_bprintf(bp, "%.6f", s->var_values[VAR_T]); +av_bprintf(bp, "%.6f", pts); } else if (!strcmp(fmt, "hms")) { if (isnan(pts)) { av_bprintf(bp, " ??:??:??.???"); -- 2.5.4 (Apple Git-61) ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 3/3] lavf/mpjpeg: do not include CRLF preceding boundary as, part of the returned frame
Thanks - any problems with 2/3? Or is it still pending? On 2/14/16 8:53 AM, Michael Niedermayer wrote: On Sat, Feb 13, 2016 at 11:49:06PM -0500, Alexander Agranovsky wrote: mpjpegdec.c |4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) 0c761f422ba83446d20728449c0c6813e4e11560 0003-lavf-mpjpeg-do-not-include-CRLF-preceding-boundary-a.patch From b2ef061db32933e896dbeecb68268a62c47d6e0a Mon Sep 17 00:00:00 2001 From: Alex Agranovsky Date: Sat, 13 Feb 2016 23:16:45 -0500 Subject: [PATCH 3/3] lavf/mpjpeg: do not include CRLF preceding boundary as part of the returned frame applied thanks [...] ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH 3/3] lavf/mpjpeg: do not include CRLF preceding boundary as, part of the returned frame
From b2ef061db32933e896dbeecb68268a62c47d6e0a Mon Sep 17 00:00:00 2001 From: Alex Agranovsky Date: Sat, 13 Feb 2016 23:16:45 -0500 Subject: [PATCH 3/3] lavf/mpjpeg: do not include CRLF preceding boundary as part of the returned frame Signed-off-by: Alex Agranovsky --- libavformat/mpjpegdec.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/libavformat/mpjpegdec.c b/libavformat/mpjpegdec.c index 2a46671..9d539bb 100644 --- a/libavformat/mpjpegdec.c +++ b/libavformat/mpjpegdec.c @@ -351,8 +351,8 @@ static int mpjpeg_read_packet(AVFormatContext *s, AVPacket *pkt) do { if (!memcmp(start, mpjpeg->searchstr, mpjpeg->searchstr_len)) { // got the boundary! rewind the stream -avio_seek(s->pb, -(len-2), SEEK_CUR); -pkt->size -= (len-2); +avio_seek(s->pb, -len, SEEK_CUR); +pkt->size -= len; return pkt->size; } len--; -- 2.5.4 (Apple Git-61) ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH 2/3] lavf/mpjpeg: probe should not depend on Content-Length
From 83c0b5f7a64e4c47801ee2f94635eea5ea144eb1 Mon Sep 17 00:00:00 2001 From: Alex Agranovsky Date: Sat, 13 Feb 2016 23:15:20 -0500 Subject: [PATCH 2/3] lavf/mpjpeg: probe should not depend on Content-Length MIME header being present Signed-off-by: Alex Agranovsky --- libavformat/mpjpegdec.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libavformat/mpjpegdec.c b/libavformat/mpjpegdec.c index 3446f2a..2a46671 100644 --- a/libavformat/mpjpegdec.c +++ b/libavformat/mpjpegdec.c @@ -124,7 +124,7 @@ static int mpjpeg_read_probe(AVProbeData *p) if (!pb) return 0; -ret = (parse_multipart_header(pb, &size, "--", NULL) > 0) ? AVPROBE_SCORE_MAX : 0; +ret = (parse_multipart_header(pb, &size, "--", NULL) >= 0) ? AVPROBE_SCORE_MAX : 0; av_free(pb); -- 2.5.4 (Apple Git-61) ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH 1/3] lavf/mpjpeg: Trim quotes on MIME boundary, if present.
From f2d8bc033cc6ab6e9a42472c99bdfce8305a13db Mon Sep 17 00:00:00 2001 From: Alex Agranovsky Date: Fri, 12 Feb 2016 12:59:29 -0500 Subject: [PATCH 1/3] lavf/mpjpeg: Trim quotes on MIME boundary, if present. Fixes 5023 Signed-off-by: Alex Agranovsky --- libavformat/mpjpegdec.c | 7 +++ 1 file changed, 7 insertions(+) diff --git a/libavformat/mpjpegdec.c b/libavformat/mpjpegdec.c index c9fcf47..3446f2a 100644 --- a/libavformat/mpjpegdec.c +++ b/libavformat/mpjpegdec.c @@ -277,6 +277,13 @@ static char* mpjpeg_get_boundary(AVIOContext* pb) len = end - start - 1; else len = strlen(start); + +/* some endpoints may enclose the boundary + in Content-Type in quotes */ +if ( len>2 && *start == '"' && start[len-1] == '"' ) { +start++; +len -= 2; +} res = av_strndup(start, len); break; } -- 2.5.4 (Apple Git-61) ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH] lavf/mpjpegdec: Fixed dereference after null check
From 36596b444e584eb7ccceb24c838f0273116691f8 Mon Sep 17 00:00:00 2001 From: Alex Agranovsky Date: Wed, 9 Dec 2015 12:48:20 -0500 Subject: [PATCH] lavf/mpjpegdec: Fixed dereference after null check Fixes Coverity CID 1341576 --- libavformat/mpjpegdec.c | 6 -- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/libavformat/mpjpegdec.c b/libavformat/mpjpegdec.c index 3429d19..dd31f87 100644 --- a/libavformat/mpjpegdec.c +++ b/libavformat/mpjpegdec.c @@ -260,8 +260,10 @@ static char* mpjpeg_get_boundary(AVIOContext* pb) start = mime_type; while (start != NULL && *start != '\0') { start = strchr(start, ';'); -if (start) -start = start+1; +if (!start) +break; + +start = start+1; while (av_isspace(*start)) start++; -- 2.4.9 (Apple Git-60) ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] Allow mpjpeg demuxer to process MIME parts which do not include Content-Length header.
On 11/30/15 10:35 AM, Alexander Agranovsky wrote: On 11/30/15 10:24 AM, Nicolas George wrote: Le decadi 10 frimaire, an CCXXIV, Alexander Agranovsky a écrit : Without getting into a religious debate This is the reason I gave practical argument. As pointed later in the thread, lu is used elsewhere. And yes, MS makes it interesting in this case. wm4 explained that point. Really, long and short should only ever be used by libc headers to implement intXX_t. I've pondered the change, but with if (!av_stristart(start, "boundary=")) { start += 9; '9' seems a bit random, and with This is the reason for the third argument of av_stristart(): if (av_stristart(start, "boundary=", &start)) { // no need to add 9, av_stristart() does it } Ah, missed that. Thanks -- attaching new iteration with this change. Regards, ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel Hi guys -- where do we stand with this? Are there any additional comments? - A. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] Allow mpjpeg demuxer to process MIME parts which do not include Content-Length header.
On 11/30/15 10:24 AM, Nicolas George wrote: Le decadi 10 frimaire, an CCXXIV, Alexander Agranovsky a écrit : Without getting into a religious debate This is the reason I gave practical argument. As pointed later in the thread, lu is used elsewhere. And yes, MS makes it interesting in this case. wm4 explained that point. Really, long and short should only ever be used by libc headers to implement intXX_t. I've pondered the change, but with if (!av_stristart(start, "boundary=")) { start += 9; '9' seems a bit random, and with This is the reason for the third argument of av_stristart(): if (av_stristart(start, "boundary=", &start)) { // no need to add 9, av_stristart() does it } Ah, missed that. Thanks -- attaching new iteration with this change. Regards, ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel From 3edc50fc36e6abffaa7ae39d1047537cd17aad62 Mon Sep 17 00:00:00 2001 From: Alex Agranovsky Date: Sun, 29 Nov 2015 18:36:20 -0500 Subject: [PATCH 1/2] avformat/mpjpeg: allow processing of MIME parts without Content-Length header Fixes ticket 5023 Signed-off-by: Alex Agranovsky --- libavformat/mpjpegdec.c | 168 +++- 1 file changed, 125 insertions(+), 43 deletions(-) diff --git a/libavformat/mpjpegdec.c b/libavformat/mpjpegdec.c index 2749a48..9d5700a 100644 --- a/libavformat/mpjpegdec.c +++ b/libavformat/mpjpegdec.c @@ -23,13 +23,30 @@ #include "avformat.h" #include "internal.h" +#include "avio_internal.h" -static int get_line(AVIOContext *pb, char *line, int line_size) + + +typedef struct MPJPEGDemuxContext { +char *boundary; +char *searchstr; +int searchstr_len; +} MPJPEGDemuxContext; + + +static void trim_right(char *p) { -int i = ff_get_line(pb, line, line_size); +if (!p || !*p) +return; + +char *end = p + strlen(p); +while (end > p && av_isspace(*(end-1))) +*(--end) = '\0'; +} -if (i > 1 && line[i - 2] == '\r') -line[i - 2] = '\0'; +static int get_line(AVIOContext *pb, char *line, int line_size) +{ +ff_get_line(pb, line, line_size); if (pb->error) return pb->error; @@ -37,21 +54,11 @@ static int get_line(AVIOContext *pb, char *line, int line_size) if (pb->eof_reached) return AVERROR_EOF; +trim_right(line); return 0; } -static void trim_right(char* p) -{ -char *end; -if (!p || !*p) -return; -end = p + strlen(p) - 1; -while (end != p && av_isspace(*end)) { -*end = '\0'; -end--; -} -} static int split_tag_value(char **tag, char **value, char *line) { @@ -86,12 +93,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 +119,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; av_free(pb); @@ -110,14 +129,15 @@ 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; - -ret = get_line(s->pb, boundary, sizeof(boundary)); -if (ret < 0) -return ret; +do { +ret = get_line(s->pb, boundary, sizeof(boundary)); +if (ret < 0) +return ret; +} while (!boundary[0]); if (strncmp(boundary, "--", 2)) return AVERROR_INVALIDDATA; @@ -147,11 +167,16 @@ 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, +
Re: [FFmpeg-devel] [PATCH] Allow mpjpeg demuxer to process MIME parts which do not include Content-Length header.
On 11/30/15 7:34 AM, wm4 wrote: On Mon, 30 Nov 2015 07:27:14 -0500 Alexander Agranovsky wrote: On 11/30/15 2:41 AM, Nicolas George wrote: Le nonidi 9 frimaire, an CCXXIV, Alexander Agranovsky a écrit : Please see the updated patches attached. The trimming loop that was subject of the discussion had been rewritten to use indices rather than pointer arithmetics. This kind of drastic change was not necessary, you can do the same with pointers. IMHO, the best way of dealing with that situation is this: when dealing with the end of the string, have the pointer point AFTER the end of the string, i.e.: char *p = string + strlen(string); // not -1 if (p > string && av_isspace(p[-1])) *(--p) = 0; That works too. +char* boundary; Here and in all new code, please use "char *var" instead of "char* var" for consistency. There is a good reason for that: "char* a, b" means that a is a pointer and b is not, grouping the pointer mark with the type is misleading. Without getting into a religious debate, not my favorite part of the style ;) Will change the code to match the style of existing, though. +"Expected boundary '%s' not found, instead found a line of %lu bytes\n", +expected_boundary, +strlen(line)); "%lu" is not correct for size_t. The correct type would be %zu, but it is possible that we have to use another construct to avoid bugs from microsoft libraries, see other instances in the code for examples. As pointed later in the thread, lu is used elsewhere. And yes, MS makes it interesting in this case. No, I meant %zu. lu really isn't safe. It will fail on 64 bit Windows, for one. (But what Nicolas meant was that %zu could fail on Windows because Windows is stuck in the past, but we work that around if necessary - I think.) -size = parse_content_length(value); -if (size < 0) -return size; +*size = parse_content_length(value); Did you remove the check on purpose? Yes. Invalid Content-Length, just as no Content-Length at all, should not prevent us from processing the part. Probably not a good idea to ignore when the server sends us definitely broken data. I'd prefer failure or at least an error message. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel And one more round ... From 3edc50fc36e6abffaa7ae39d1047537cd17aad62 Mon Sep 17 00:00:00 2001 From: Alex Agranovsky Date: Sun, 29 Nov 2015 18:36:20 -0500 Subject: [PATCH 1/2] avformat/mpjpeg: allow processing of MIME parts without Content-Length header Fixes ticket 5023 Signed-off-by: Alex Agranovsky --- libavformat/mpjpegdec.c | 168 +++- 1 file changed, 125 insertions(+), 43 deletions(-) diff --git a/libavformat/mpjpegdec.c b/libavformat/mpjpegdec.c index 2749a48..9d5700a 100644 --- a/libavformat/mpjpegdec.c +++ b/libavformat/mpjpegdec.c @@ -23,13 +23,30 @@ #include "avformat.h" #include "internal.h" +#include "avio_internal.h" -static int get_line(AVIOContext *pb, char *line, int line_size) + + +typedef struct MPJPEGDemuxContext { +char *boundary; +char *searchstr; +int searchstr_len; +} MPJPEGDemuxContext; + + +static void trim_right(char *p) { -int i = ff_get_line(pb, line, line_size); +if (!p || !*p) +return; + +char *end = p + strlen(p); +while (end > p && av_isspace(*(end-1))) +*(--end) = '\0'; +} -if (i > 1 && line[i - 2] == '\r') -line[i - 2] = '\0'; +static int get_line(AVIOContext *pb, char *line, int line_size) +{ +ff_get_line(pb, line, line_size); if (pb->error) return pb->error; @@ -37,21 +54,11 @@ static int get_line(AVIOContext *pb, char *line, int line_size) if (pb->eof_reached) return AVERROR_EOF; +trim_right(line); return 0; } -static void trim_right(char* p) -{ -char *end; -if (!p || !*p) -return; -end = p + strlen(p) - 1; -while (end != p && av_isspace(*end)) { -*end = '\0'; -end--; -} -} static int split_tag_value(char **tag, char **value, char *line) { @@ -86,12 +93,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
Re: [FFmpeg-devel] [PATCH] Allow mpjpeg demuxer to process MIME parts which do not include Content-Length header.
On 11/30/15 7:34 AM, wm4 wrote: On Mon, 30 Nov 2015 07:27:14 -0500 Alexander Agranovsky wrote: On 11/30/15 2:41 AM, Nicolas George wrote: Le nonidi 9 frimaire, an CCXXIV, Alexander Agranovsky a écrit : Please see the updated patches attached. The trimming loop that was subject of the discussion had been rewritten to use indices rather than pointer arithmetics. This kind of drastic change was not necessary, you can do the same with pointers. IMHO, the best way of dealing with that situation is this: when dealing with the end of the string, have the pointer point AFTER the end of the string, i.e.: char *p = string + strlen(string); // not -1 if (p > string && av_isspace(p[-1])) *(--p) = 0; That works too. +char* boundary; Here and in all new code, please use "char *var" instead of "char* var" for consistency. There is a good reason for that: "char* a, b" means that a is a pointer and b is not, grouping the pointer mark with the type is misleading. Without getting into a religious debate, not my favorite part of the style ;) Will change the code to match the style of existing, though. +"Expected boundary '%s' not found, instead found a line of %lu bytes\n", +expected_boundary, +strlen(line)); "%lu" is not correct for size_t. The correct type would be %zu, but it is possible that we have to use another construct to avoid bugs from microsoft libraries, see other instances in the code for examples. As pointed later in the thread, lu is used elsewhere. And yes, MS makes it interesting in this case. No, I meant %zu. lu really isn't safe. It will fail on 64 bit Windows, for one. (But what Nicolas meant was that %zu could fail on Windows because Windows is stuck in the past, but we work that around if necessary - I think.) -size = parse_content_length(value); -if (size < 0) -return size; +*size = parse_content_length(value); Did you remove the check on purpose? Yes. Invalid Content-Length, just as no Content-Length at all, should not prevent us from processing the part. Probably not a good idea to ignore when the server sends us definitely broken data. I'd prefer failure or at least an error message. I'll add a warning, how about that? ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] Allow mpjpeg demuxer to process MIME parts which do not include Content-Length header.
On 11/30/15 6:37 AM, wm4 wrote: On Mon, 30 Nov 2015 08:41:42 +0100 Nicolas George wrote: +"Expected boundary '%s' not found, instead found a line of %lu bytes\n", +expected_boundary, +strlen(line)); "%lu" is not correct for size_t. The correct type would be %zu, but it is possible that we have to use another construct to avoid bugs from microsoft libraries, see other instances in the code for examples. It's used in some portable code (e.g. http.c), so it's probably fine. -size = parse_content_length(value); -if (size < 0) -return size; +*size = parse_content_length(value); Did you remove the check on purpose? He probably did, but now that I look at this again, it seems a bit shady. This code parses the Content-length header, and if parsing it fails, it will now try the boundary instead. But doesn't this header always require a numeric value if it's present at all? It surely does. However, seeing creative ways in which existing endpoints choose to break MIME, I'd rather not introduce a restriction where we don't have to. Take the boundary for example -- the code before my changes (and after as well, at least by default) only looks for "CRLF--". I would've found it unfathomable and unacceptable, if I didn't recently see a trace from a popular consumer camera, where one boundary was declared, and another was used. I'm a huge proponent of standards *compliance*, but when standards *enforcement* will result in a regression report, even if the other side is at fault, choosing to be a bit more flexible may be the way to go. In this particular case I don't see a downside, do you? ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] Allow mpjpeg demuxer to process MIME parts which do not include Content-Length header.
On 11/30/15 2:41 AM, Nicolas George wrote: Le nonidi 9 frimaire, an CCXXIV, Alexander Agranovsky a écrit : Please see the updated patches attached. The trimming loop that was subject of the discussion had been rewritten to use indices rather than pointer arithmetics. This kind of drastic change was not necessary, you can do the same with pointers. IMHO, the best way of dealing with that situation is this: when dealing with the end of the string, have the pointer point AFTER the end of the string, i.e.: char *p = string + strlen(string); // not -1 if (p > string && av_isspace(p[-1])) *(--p) = 0; That works too. +char* boundary; Here and in all new code, please use "char *var" instead of "char* var" for consistency. There is a good reason for that: "char* a, b" means that a is a pointer and b is not, grouping the pointer mark with the type is misleading. Without getting into a religious debate, not my favorite part of the style ;) Will change the code to match the style of existing, though. +"Expected boundary '%s' not found, instead found a line of %lu bytes\n", +expected_boundary, +strlen(line)); "%lu" is not correct for size_t. The correct type would be %zu, but it is possible that we have to use another construct to avoid bugs from microsoft libraries, see other instances in the code for examples. As pointed later in the thread, lu is used elsewhere. And yes, MS makes it interesting in this case. -size = parse_content_length(value); -if (size < 0) -return size; +*size = parse_content_length(value); Did you remove the check on purpose? Yes. Invalid Content-Length, just as no Content-Length at all, should not prevent us from processing the part. +if (!av_strncasecmp(start, "boundary=", 9)) { +start += 9; It has already be pointed out: av_stristart() to avoid the duplicated magic number. I've pondered the change, but with if (!av_stristart(start, "boundary=")) { start += 9; '9' seems a bit random, and with if (!av_stristart(start, "boundary=")) { start += strlen("boundary="); we end up with unnecessarily taking strlen at least twice. Again, not critical, but IMHO as written it is the best compromise between readability and efficiency. Thanks for your comments! Can not comment on the functional aspect, sorry. Regards, ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] Allow mpjpeg demuxer to process MIME parts which do not include Content-Length header.
On 11/29/15 1:16 PM, Nicolas George wrote: Le nonidi 9 frimaire, an CCXXIV, Ganesh Ajjanagadde a écrit : end = p + strlen(p) - 1; Don't know what you are referring to here, but dereferencing is clearly invalid. However, in order to allow common loop idioms, pointer arithmetic one element beyond a array (memory) range is valid: Of course. But one element BEFORE the beginning of an array is invalid. In other words p + sizeof(p) is valid (assuming p is char[]), but p - 1 is not. In this instance, if p is a 0-terminated string, p + strlen(p) is always valid, even without the provision you mention, because the 0 is part of the object. But p + strlen(p) - 1 is not, if strlen(p) can be 0. Note that dereferencing or not is not relevant: some architectures have special registers for pointers that would cause traps just loading an invalid pointer (think: p at offset 0 of segment S, p-1 at offset max of segment S-1 -> load segment descriptor). FFmpeg probably does not run on any such architecture, though. But still, this is bad style. Regards, ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel Ended up reworking these few lines to operate on indices, rather than doing pointer airthmetics. Should be the same thing, functionally. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] Allow mpjpeg demuxer to process MIME parts which do not include Content-Length header.
On 11/29/15 1:00 PM, wm4 wrote: On Wed, 25 Nov 2015 10:35:31 -0500 Alex Agranovsky wrote: From 4797590e11267993c3883e5037619625e1f1dadf Mon Sep 17 00:00:00 2001 From: Alex Agranovsky Date: Tue, 24 Nov 2015 00:07:34 -0500 Subject: [PATCH 2/2] If available, use the actual boundary in HTTP response's Content-Type header, to separate MIME parts in mpjpeg stream This code is disabled by default so not to regress endpoints sending invalid MIME, but can be enabled via AVOption 'strict_mime_boundary' Signed-off-by: Alex Agranovsky --- libavformat/mpjpegdec.c | 76 +++-- 1 file changed, 73 insertions(+), 3 deletions(-) diff --git a/libavformat/mpjpegdec.c b/libavformat/mpjpegdec.c index b9093ea..c0f011d 100644 --- a/libavformat/mpjpegdec.c +++ b/libavformat/mpjpegdec.c @@ -20,6 +20,7 @@ */ #include "libavutil/avstring.h" +#include "libavutil/opt.h" #include "avformat.h" #include "internal.h" @@ -28,9 +29,11 @@ typedef struct MPJPEGDemuxContext { +const AVClass *class; char* boundary; char* searchstr; int searchstr_len; +int strict_mime_boundary; } MPJPEGDemuxContext; @@ -245,6 +248,43 @@ static int parse_multipart_header(AVIOContext *pb, } +static char* mpjpeg_get_boundary(AVIOContext* pb) +{ +uint8_t *mime_type = NULL; +uint8_t *start; +uint8_t *end; +uint8_t *res = NULL; +int len; + +/* get MIME type, and skip to the first parameter */ +av_opt_get(pb, "mime_type", AV_OPT_SEARCH_CHILDREN, &mime_type); +start = mime_type; +while (start != NULL && *start != '\0') { +start = strchr(start, ';'); +if (start) +start = start+1; + +while (av_isspace(*start)) +start++; + +if (!av_strncasecmp(start, "boundary=", 9)) { +start += 9; (Probably could use av_strstart() here, but you don't need to change it if you don't want to.) + +end = strchr(start, ';'); +if (end) +len = end - start - 1; +else +len = strlen(start); +res = av_strndup(start, len); +break; +} +} + +av_freep(&mime_type); +return res; +} + + static int mpjpeg_read_packet(AVFormatContext *s, AVPacket *pkt) { int size; @@ -252,8 +292,19 @@ static int mpjpeg_read_packet(AVFormatContext *s, AVPacket *pkt) MPJPEGDemuxContext *mpjpeg = s->priv_data; if (mpjpeg->boundary == NULL) { -mpjpeg->boundary = av_strdup("--"); -mpjpeg->searchstr = av_strdup("\r\n--"); +uint8_t* boundary = NULL; +if (mpjpeg->strict_mime_boundary) { +boundary = mpjpeg_get_boundary(s->pb); +} +if (boundary != NULL) { +size_t bufsize = 4+strlen(boundary)+1; +mpjpeg->boundary = boundary; +mpjpeg->searchstr = av_malloc(bufsize); +snprintf( mpjpeg->searchstr, bufsize, "\r\n%s\r\n", boundary ); av_asprintf() would be much more convenient. +} else { +mpjpeg->boundary = av_strdup("--"); +mpjpeg->searchstr = av_strdup("\r\n--"); +} mpjpeg->searchstr_len = strlen(mpjpeg->searchstr); if (!mpjpeg->boundary || !mpjpeg->searchstr) { av_freep(&mpjpeg->boundary); @@ -315,6 +366,22 @@ static int mpjpeg_read_packet(AVFormatContext *s, AVPacket *pkt) return ret; } +#define OFFSET(x) offsetof(MPJPEGDemuxContext, x) + +#define DEC AV_OPT_FLAG_DECODING_PARAM +const AVOption mpjpeg_options[] = { +{ "strict_mime_boundary", "require MIME boundaries match", OFFSET(strict_mime_boundary), AV_OPT_TYPE_INT, {.i64 = 0}, 0, 1, DEC }, Not sure, but maybe it'd be good to have a better explanation for this option in doc/demuxers.texi. +{ NULL } +}; + + +static const AVClass mpjpeg_demuxer_class = { +.class_name = "MPJPEG demuxer", +.item_name = av_default_item_name, +.option = mpjpeg_options, +.version= LIBAVUTIL_VERSION_INT, +}; + AVInputFormat ff_mpjpeg_demuxer = { .name = "mpjpeg", .long_name = NULL_IF_CONFIG_SMALL("MIME multipart JPEG"), @@ -324,5 +391,8 @@ AVInputFormat ff_mpjpeg_demuxer = { .read_probe= mpjpeg_read_probe, .read_header = mpjpeg_read_header, .read_packet = mpjpeg_read_packet, -.read_close= mpjpeg_read_close +.read_close= mpjpeg_read_close, +.priv_class= &mpjpeg_demuxer_class }; + + Rest looks good to me. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel Please see the updated patches attached. The trimming loop that was subject of the discussion had been rewritten to use indices rather than pointer arithmetics. Best, - Alex From 7d9fdaee79f1cd671aa62799b