Re: [FFmpeg-devel] [PATCH] Allow mpjpeg demuxer to process MIME parts which do not include Content-Length header.

2015-12-02 Thread Alexander Agranovsky 



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=", )) {
// 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.

2015-12-02 Thread wm4 
On Wed, 2 Dec 2015 09:28:40 -0500
Alexander Agranovsky  wrote:

> Hi guys -- where do we stand with this? Are there any additional comments?
> 
> - A.

Looks fine now. The trim_right() still looks a bit awkward IMHO, but it
should be correct (whoever really cares can send a follow-up patch to
change it). I'll apply this tonight, I guess.
___
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.

2015-12-02 Thread wm4 
On Wed, 2 Dec 2015 09:28:40 -0500
Alexander Agranovsky  wrote:


> Hi guys -- where do we stand with this? Are there any additional comments?
> 
> - A.

Pushed both patches.
___
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.

2015-11-30 Thread Nicolas George 
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=", )) {
// no need to add 9, av_stristart() does it
}

Regards,

-- 
  Nicolas George


signature.asc
Description: Digital signature
___
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.

2015-11-30 Thread Alexander Agranovsky 



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=", )) {
// 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(>boundary);
+av_freep(>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, , "--", 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,
+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));
@@

Re: [FFmpeg-devel] [PATCH] Allow mpjpeg demuxer to process MIME parts which do not include Content-Length header.

2015-11-30 Thread wm4 
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?
___
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.

2015-11-30 Thread Alexander Agranovsky 



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.

2015-11-30 Thread wm4 
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

Re: [FFmpeg-devel] [PATCH] Allow mpjpeg demuxer to process MIME parts which do not include Content-Length header.

2015-11-30 Thread Alexander Agranovsky 



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.

2015-11-30 Thread Alexander Agranovsky 



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.

2015-11-30 Thread Alexander Agranovsky 



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(>boundary);
+av_freep(>searchstr);
+return 0;
+}
 
 static int mpjpeg_read_probe(AVProbeData *p)
 {
 AVIOContext *pb;
 int

Re: [FFmpeg-devel] [PATCH] Allow mpjpeg demuxer to process MIME parts which do not include Content-Length header.

2015-11-29 Thread wm4 
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, _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(_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(>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= _demuxer_class
>  };
> +
> +

Rest looks good to me.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org

Re: [FFmpeg-devel] [PATCH] Allow mpjpeg demuxer to process MIME parts which do not include Content-Length header.

2015-11-29 Thread wm4 
On Wed, 25 Nov 2015 10:35:31 -0500
Alex Agranovsky  wrote:

> From 70a6e1b0f3d47698bf49c3c766d5472646bff71a Mon Sep 17 00:00:00 2001
> From: Alex Agranovsky 
> 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.

Commit messages should start with a prefix (like "avformat/mpjpeg: "),
and then maybe up to 60 characters of summary. If more text is needed,
it should be part of the commit body (i.e. the part after the subject
line).

(Same for the second patch.)

> 
> Fixes ticket 5023
> 
> Signed-off-by: Alex Agranovsky 
> ---
>  libavformat/mpjpegdec.c | 164 
> +---
>  1 file changed, 126 insertions(+), 38 deletions(-)
> 
> diff --git a/libavformat/mpjpegdec.c b/libavformat/mpjpegdec.c
> index 2749a48..b9093ea 100644
> --- a/libavformat/mpjpegdec.c
> +++ b/libavformat/mpjpegdec.c
> @@ -23,22 +23,15 @@
>  
>  #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;
> -}
> +typedef struct MPJPEGDemuxContext {
> +char*   boundary;
> +char*   searchstr;
> +int searchstr_len;
> +} MPJPEGDemuxContext;
>  
>  
>  static void trim_right(char* p)
> @@ -47,12 +40,28 @@ static void trim_right(char* p)
>  if (!p || !*p)
>  return;
>  end = p + strlen(p) - 1;
> -while (end != p && av_isspace(*end)) {
> +while (end >= p && av_isspace(*end)) {

I still think that this change is theoretically unclean and invalid,
because end will point before the memory location at one point. Maybe
someone else can give a second opinion whether it's invalid or allowed
by the C standard, and whether it's ok to ignore it. (Besides writing a
valid trim shouldn't be hard.)

>  *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 +95,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(>boundary);
> +av_freep(>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 +121,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, , "--", NULL) > 0) ? 
> AVPROBE_SCORE_MAX : 0;
>  
>  av_free(pb);
>  
> @@ -110,14 +131,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 +169,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,
> +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));
> @@ -161,14 +188,23 @@ static int

Re: [FFmpeg-devel] [PATCH] Allow mpjpeg demuxer to process MIME parts which do not include Content-Length header.

2015-11-29 Thread Ganesh Ajjanagadde 
On Sun, Nov 29, 2015 at 1:00 PM, wm4  wrote:
> On Wed, 25 Nov 2015 10:35:31 -0500
> Alex Agranovsky  wrote:
>
>> From 70a6e1b0f3d47698bf49c3c766d5472646bff71a Mon Sep 17 00:00:00 2001
>> From: Alex Agranovsky 
>> 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.
>
> Commit messages should start with a prefix (like "avformat/mpjpeg: "),
> and then maybe up to 60 characters of summary. If more text is needed,
> it should be part of the commit body (i.e. the part after the subject
> line).
>
> (Same for the second patch.)
>
>>
>> Fixes ticket 5023
>>
>> Signed-off-by: Alex Agranovsky 
>> ---
>>  libavformat/mpjpegdec.c | 164 
>> +---
>>  1 file changed, 126 insertions(+), 38 deletions(-)
>>
>> diff --git a/libavformat/mpjpegdec.c b/libavformat/mpjpegdec.c
>> index 2749a48..b9093ea 100644
>> --- a/libavformat/mpjpegdec.c
>> +++ b/libavformat/mpjpegdec.c
>> @@ -23,22 +23,15 @@
>>
>>  #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;
>> -}
>> +typedef struct MPJPEGDemuxContext {
>> +char*   boundary;
>> +char*   searchstr;
>> +int searchstr_len;
>> +} MPJPEGDemuxContext;
>>
>>
>>  static void trim_right(char* p)
>> @@ -47,12 +40,28 @@ static void trim_right(char* p)
>>  if (!p || !*p)
>>  return;
>>  end = p + strlen(p) - 1;
>> -while (end != p && av_isspace(*end)) {
>> +while (end >= p && av_isspace(*end)) {
>
> I still think that this change is theoretically unclean and invalid,
> because end will point before the memory location at one point. Maybe
> someone else can give a second opinion whether it's invalid or allowed
> by the C standard, and whether it's ok to ignore it. (Besides writing a
> valid trim shouldn't be hard.)

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:
https://stackoverflow.com/questions/8133804/negative-array-index-in-c.

[...]
___
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.

2015-11-29 Thread Alexander Agranovsky 



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, _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(_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(>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= _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.

Re: [FFmpeg-devel] [PATCH] Allow mpjpeg demuxer to process MIME parts which do not include Content-Length header.

2015-11-29 Thread Alexander Agranovsky 



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.

2015-11-29 Thread Nicolas George 
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;

> +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.

> +"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.

> -size = parse_content_length(value);
> -if (size < 0)
> -return size;
> +*size = parse_content_length(value);

Did you remove the check on purpose?

> +if (!av_strncasecmp(start, "boundary=", 9)) {
> +start += 9;

It has already be pointed out: av_stristart() to avoid the duplicated magic
number.

Can not comment on the functional aspect, sorry.

Regards,

-- 
  Nicolas George


signature.asc
Description: Digital signature
___
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.

2015-11-29 Thread Nicolas George 
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,

-- 
  Nicolas George


signature.asc
Description: Digital signature
___
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.

2015-11-27 Thread wm4 
On Fri, 27 Nov 2015 15:14:54 -0500
Alex Agranovsky  wrote:

> 
> Hi - are there any additional corrections I need to address on this set of 
> patches, or is it good to go at this point?

Sorry, forgot about it, will look tomorrow.

Also, the quoting in your replies is completely messed up. I can't tell
what is text from others and what you have written.
___
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.

2015-11-27 Thread Alex Agranovsky 
On November 25, 2015 at 10:35:33 AM, Alex Agranovsky (a...@sighthound.com) 
wrote:

On November 24, 2015 at 6:06:36 PM, Michael Niedermayer (michae...@gmx.at) 
wrote:

On Tue, Nov 24, 2015 at 03:01:28PM -0500, Alex Agranovsky wrote: 
[...] 

> From 2c253d7978a6c9c2dc701d393eb5b9d68e831c98 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 break some fragile endpoints 
> sending invalid MIME, but can be enabled via AVOption 'strict_mime_boundary' 
> 
> Signed-off-by: Alex Agranovsky  
> --- 
> libavformat/mpjpegdec.c | 75 
> +++-- 
> 1 file changed, 72 insertions(+), 3 deletions(-) 
> 
> diff --git a/libavformat/mpjpegdec.c b/libavformat/mpjpegdec.c 
> index b9093ea..b89f128 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, _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; 
> + 
> + end = strchr(start, ';'); 
> + if (end) 
> + len = end - start - 1; 
> + else 
> + len = strlen(start); 
> + res = av_strndup(start, len); 
> + break; 
> + } 
> + } 
> + 
> + av_freep(_type); 
> + return res; 
> +} 
> + 
> + 
> static int mpjpeg_read_packet(AVFormatContext *s, AVPacket *pkt) 
> { 
> int size; 
> @@ -252,8 +292,18 @@ 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) { 
> + mpjpeg->boundary = boundary; 
> + mpjpeg->searchstr = av_malloc(4+strlen(boundary)+1); 

> + sprintf( mpjpeg->searchstr, "\r\n%s\r\n", boundary ); 

please use snprintf() 


> + } else { 
> + mpjpeg->boundary = av_strdup("--"); 
> + mpjpeg->searchstr = av_strdup("\r\n--"); 
> + } 
> mpjpeg->searchstr_len = strlen(mpjpeg->searchstr); 
> if (!mpjpeg->boundary || !mpjpeg->searchstr) { 
> av_freep(>boundary); 
> @@ -315,6 +365,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 }, 
> + { NULL } 
> +}; 
> + 
> + 

> +static const AVClass ff_mpjpeg_demuxer_class = { 

there should be no ff_ as its static 


> + .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 +390,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 
> }; 
> + 
> + 
> -- 
> 2.4.9 (Apple Git-60) 
> 

> ___ 
> ffmpeg-devel mailing list 
> ffmpeg-devel@ffmpeg.org 
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel 


-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB 

Awnsering whenever a program halts or runs forever is 
On a turing machine, in general impossible (turings halting problem). 
On any real computer, always possible as a real computer has a finite number 
of states N, and will either halt in less than N cycles or never halt. 
___
ffmpeg-devel mailing list

Re: [FFmpeg-devel] [PATCH] Allow mpjpeg demuxer to process MIME parts which do not include Content-Length header.

2015-11-25 Thread Alex Agranovsky 

On November 24, 2015 at 6:06:36 PM, Michael Niedermayer (michae...@gmx.at) 
wrote:

On Tue, Nov 24, 2015 at 03:01:28PM -0500, Alex Agranovsky wrote: 
[...] 

> From 2c253d7978a6c9c2dc701d393eb5b9d68e831c98 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 break some fragile endpoints 
> sending invalid MIME, but can be enabled via AVOption 'strict_mime_boundary' 
> 
> Signed-off-by: Alex Agranovsky  
> --- 
> libavformat/mpjpegdec.c | 75 
> +++-- 
> 1 file changed, 72 insertions(+), 3 deletions(-) 
> 
> diff --git a/libavformat/mpjpegdec.c b/libavformat/mpjpegdec.c 
> index b9093ea..b89f128 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, _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; 
> + 
> + end = strchr(start, ';'); 
> + if (end) 
> + len = end - start - 1; 
> + else 
> + len = strlen(start); 
> + res = av_strndup(start, len); 
> + break; 
> + } 
> + } 
> + 
> + av_freep(_type); 
> + return res; 
> +} 
> + 
> + 
> static int mpjpeg_read_packet(AVFormatContext *s, AVPacket *pkt) 
> { 
> int size; 
> @@ -252,8 +292,18 @@ 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) { 
> + mpjpeg->boundary = boundary; 
> + mpjpeg->searchstr = av_malloc(4+strlen(boundary)+1); 

> + sprintf( mpjpeg->searchstr, "\r\n%s\r\n", boundary ); 

please use snprintf() 


> + } else { 
> + mpjpeg->boundary = av_strdup("--"); 
> + mpjpeg->searchstr = av_strdup("\r\n--"); 
> + } 
> mpjpeg->searchstr_len = strlen(mpjpeg->searchstr); 
> if (!mpjpeg->boundary || !mpjpeg->searchstr) { 
> av_freep(>boundary); 
> @@ -315,6 +365,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 }, 
> + { NULL } 
> +}; 
> + 
> + 

> +static const AVClass ff_mpjpeg_demuxer_class = { 

there should be no ff_ as its static 


> + .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 +390,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 
> }; 
> + 
> + 
> -- 
> 2.4.9 (Apple Git-60) 
> 

> ___ 
> ffmpeg-devel mailing list 
> ffmpeg-devel@ffmpeg.org 
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel 


-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB 

Awnsering whenever a program halts or runs forever is 
On a turing machine, in general impossible (turings halting problem). 
On any real computer, always possible as a real computer has a finite number 
of states N, and will either halt in less than N cycles or never halt. 
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel


Please find the patches

Re: [FFmpeg-devel] [PATCH] Allow mpjpeg demuxer to process MIME parts which do not include Content-Length header.

2015-11-24 Thread Michael Niedermayer 
On Tue, Nov 24, 2015 at 03:01:28PM -0500, Alex Agranovsky wrote:
[...]

> From 2c253d7978a6c9c2dc701d393eb5b9d68e831c98 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 break some fragile endpoints 
> sending invalid MIME, but can be enabled via AVOption 'strict_mime_boundary'
> 
> Signed-off-by: Alex Agranovsky 
> ---
>  libavformat/mpjpegdec.c | 75 
> +++--
>  1 file changed, 72 insertions(+), 3 deletions(-)
> 
> diff --git a/libavformat/mpjpegdec.c b/libavformat/mpjpegdec.c
> index b9093ea..b89f128 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, _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;
> +
> +end = strchr(start, ';');
> +if (end)
> +len = end - start - 1;
> +else
> +len = strlen(start);
> +res = av_strndup(start, len);
> +break;
> +}
> +}
> +
> +av_freep(_type);
> +return res;
> +}
> +
> +
>  static int mpjpeg_read_packet(AVFormatContext *s, AVPacket *pkt)
>  {
>  int size;
> @@ -252,8 +292,18 @@ 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) {
> +mpjpeg->boundary = boundary;
> +mpjpeg->searchstr = av_malloc(4+strlen(boundary)+1);

> +sprintf( mpjpeg->searchstr, "\r\n%s\r\n", boundary );

please use snprintf()


> +} else {
> +mpjpeg->boundary = av_strdup("--");
> +mpjpeg->searchstr = av_strdup("\r\n--");
> +}
>  mpjpeg->searchstr_len = strlen(mpjpeg->searchstr);
>  if (!mpjpeg->boundary || !mpjpeg->searchstr) {
>  av_freep(>boundary);
> @@ -315,6 +365,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 },
> +{ NULL }
> +};
> +
> +

> +static const AVClass ff_mpjpeg_demuxer_class = {

there should be no ff_ as its static


> +.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 +390,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
>  };
> +
> +
> -- 
> 2.4.9 (Apple Git-60)
> 

> ___
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel


-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Awnsering whenever a program halts or runs forever is
On a turing machine, in general impossible

Re: [FFmpeg-devel] [PATCH] Allow mpjpeg demuxer to process MIME parts which do not include Content-Length header.

2015-11-24 Thread Alex Agranovsky 


-- 
Alex Agranovsky
Sighthound, Inc
www.sighthound.com  

On November 24, 2015 at 12:36:30 PM, wm4 (nfx...@googlemail.com) wrote:

On Tue, 24 Nov 2015 11:39:07 -0500  
Alex Agranovsky  wrote:  

> On November 24, 2015 at 10:32:47 AM, wm4 (nfx...@googlemail.com) wrote:  
>  
> On Tue, 24 Nov 2015 00:16:06 -0500  
> Alex Agranovsky  wrote:  
>  
> > From a2a0b9e0da14b6e82aa783535ec1878c8ffbede0 Mon Sep 17 00:00:00 2001  
> > From: Alex Agranovsky   
> > 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   
> > ---  
> > 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.  
> Will be removed in follow up submission.  
>  
>  
>  
> > +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.  
>  
>  
> Consider input of a single ‘\r’. It will never get trimmed with the old code. 
>  

I don't really see how most of the changes fix this case. The only  
change that does anything is replacing the != operator with >= . Which  
is invalid as I see just now: it would set end to p-1, which AFAIK is  
undefined behavior.  


Setting end to p-1 would force us to exit the loop, no? We’re just comparing 
two pointer values, without dereferencing them.
I do see that length check is redundant with !*p, so removing that.

> > *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(>boundary); 
> > + av_freep(>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, , "--", NULL) > 0 ) ? 
> > AVPROBE_SCORE_MAX : 0;  
> 
> A stray space got in. 
> Parameters to parse_multipart_header had changed.  

Yes, but the 2 additional spaces after the ( and before the ) are not 
consistent with the rest of the coding style. (Though I admit the 
FFmpeg code has some consistency problems, e.g. the line you changed 
was missing some spaces.) 

While these cosmetic issues are not that important, I think at least 
some effort should be put into not making it look worse. 
Agreed. It takes effort, though, to switch to a specific project’s style, so I 
apologize for having to go through that.  I’ll

Re: [FFmpeg-devel] [PATCH] Allow mpjpeg demuxer to process MIME parts which do not include Content-Length header.

2015-11-24 Thread Alex Agranovsky 




On November 24, 2015 at 10:32:47 AM, wm4 (nfx...@googlemail.com) wrote:

On Tue, 24 Nov 2015 00:16:06 -0500
Alex Agranovsky  wrote:

> From a2a0b9e0da14b6e82aa783535ec1878c8ffbede0 Mon Sep 17 00:00:00 2001
> From: Alex Agranovsky 
> 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 
> ---
> 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.
Will be removed in follow up submission.



> +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.


Consider input of a single ‘\r’. It will never get trimmed with the old code.

> *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(>boundary);
> + av_freep(>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, , "--", NULL) > 0 ) ? 
> AVPROBE_SCORE_MAX : 0;

A stray space got in.
Parameters to parse_multipart_header had changed. 





>  
> 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?
MIME standard, as old as it is, is poorly implemented by many camera 
manufacturers. In the last few weeks, I have seen things ranging from not 
sending a proper boundary, to not including a CRLF after a body part, to 
including multiples. When we process the headers, it is assumed body part had 
been consumed, and we need to get to the next non-empty lines. It is solving 
the same problem as 18f9308e6a96bbeb034ee5213a6d41e0b6c2ae74, just the other 
manifestation of it.





> - if (strncmp(boundary, "--", 2))
> + if (strncmp(boundary, "--", 2)) {
> return AVERROR_INVALIDDATA;
> + }

Another change that looks like a stray change.
Will remove in follow-up submission.



>  
> 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 =

Re: [FFmpeg-devel] [PATCH] Allow mpjpeg demuxer to process MIME parts which do not include Content-Length header.

2015-11-24 Thread wm4 
On Tue, 24 Nov 2015 00:16:06 -0500
Alex Agranovsky  wrote:

> From a2a0b9e0da14b6e82aa783535ec1878c8ffbede0 Mon Sep 17 00:00:00 2001
> From: Alex Agranovsky 
> 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 
> ---
>  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(>boundary);
> +av_freep(>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, , "--", 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;