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

[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 at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel