Bug 47059 - [Qt] Handle multipart images
Summary: [Qt] Handle multipart images
Status: RESOLVED INVALID
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Qt (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords: Qt, QtTriaged
Depends on:
Blocks: 47060 69615
  Show dependency treegraph
 
Reported: 2010-10-03 09:40 PDT by Robert Hogan
Modified: 2014-02-03 03:16 PST (History)
9 users (show)

See Also:


Attachments
Patch (8.09 KB, patch)
2010-10-03 12:51 PDT, Robert Hogan
no flags Details | Formatted Diff | Diff
Patch (9.51 KB, patch)
2010-10-04 14:28 PDT, Robert Hogan
no flags Details | Formatted Diff | Diff
Patch (9.63 KB, patch)
2010-10-05 12:17 PDT, Robert Hogan
no flags Details | Formatted Diff | Diff
Patch (10.52 KB, patch)
2010-10-08 13:42 PDT, Robert Hogan
no flags Details | Formatted Diff | Diff
Patch (10.54 KB, patch)
2010-10-09 11:44 PDT, Robert Hogan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Robert Hogan 2010-10-03 09:40:04 PDT
Support multipart-responses on the assumption that they are bound to an image object, so we remove headers and boundary markers and send the raw image data only.
Comment 1 Robert Hogan 2010-10-03 12:51:39 PDT
Created attachment 69593 [details]
Patch
Comment 2 Robert Hogan 2010-10-04 14:28:56 PDT
Created attachment 69680 [details]
Patch
Comment 3 Robert Hogan 2010-10-05 12:17:47 PDT
Created attachment 69826 [details]
Patch
Comment 4 Andreas Kling 2010-10-05 14:11:12 PDT
Comment on attachment 69826 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=69826&action=review

Quick r- for somewhat superficial reasons.

Peter Hartmann (p--hartmann on IRC) has been working on multipart support for QNAM, I've asked him to have a look at this.

> WebCore/platform/network/qt/QNetworkReplyHandler.cpp:432
> +    int s = contentType.lastIndexOf("boundary=") + 9;

"s" is not a good variable name.

> WebCore/platform/network/qt/QNetworkReplyHandler.cpp:446
> +        int bs = data->indexOf(boundary);

"bs" is not a good variable name.

> WebCore/platform/network/qt/QNetworkReplyHandler.h:72
> +    void removeMultipartHeaders(QByteArray* data);
> +    void processMultipartResponse(QByteArray* data, ResourceHandleClient* client);

None of the argument names (data, data and client) are needed here.

> WebCore/platform/network/qt/QNetworkReplyHandler.h:73
> +    bool isMultipartResponse();

This method should be const.
Comment 5 Robert Hogan 2010-10-05 14:31:11 PDT
I think this change can still be useful while waiting for Peter's QNAM work to land in Qt. When it does, we will still need to special-case multipart responses in QNetworkReplyHandler because of the WebCore quirks noted in the comments.

Certainly, bug 47060 will definitely need support in QNAM.
Comment 6 Peter Hartmann 2010-10-06 04:30:17 PDT
View in context: https://bugs.webkit.org/attachment.cgi?id=69826&action=review

The multipart code that we plan for Qt 4.8 is for sending multipart messages only, see http://bugreports.qt.nokia.com/browse/QTBUG-6222 .

> WebCore/platform/network/qt/QNetworkReplyHandler.cpp:424
> +    if (contentType.contains("multipart/x-mixed-replace"))

shouldn't we check for contentType.startsWith("multipart/") ?

>> WebCore/platform/network/qt/QNetworkReplyHandler.cpp:432
>> +    int s = contentType.lastIndexOf("boundary=") + 9;
>
> "s" is not a good variable name.

the boundary could (and in fact, should) be quoted, recommended in RFC 2046 section 5.1.1; I think the patch should handle that use case as well

>> WebCore/platform/network/qt/QNetworkReplyHandler.cpp:446
>> +        int bs = data->indexOf(boundary);
>
> "bs" is not a good variable name.

maybe combine the two above lines to

while ((boundaryIndex = data->indexOf(boundary)) != -1) {

the indexOf() method could be costly for big parts

> WebCore/platform/network/qt/QNetworkReplyHandler.cpp:450
> +        data->replace(part, "");

how about keeping data constant and only use the char pointer and indexing? data might be a really big byte array, so detaching should be avoided. Or how about wrapping it into a QBuffer? Or could it somehow be avoided to construct the QByteArray in the first place, and operate on some char pointer instead?
Comment 7 Robert Hogan 2010-10-06 13:37:12 PDT
(In reply to comment #6)
> View in context: https://bugs.webkit.org/attachment.cgi?id=69826&action=review
> 
> The multipart code that we plan for Qt 4.8 is for sending multipart messages only, see http://bugreports.qt.nokia.com/browse/QTBUG-6222 .
> 
> > WebCore/platform/network/qt/QNetworkReplyHandler.cpp:424
> > +    if (contentType.contains("multipart/x-mixed-replace"))
> 
> shouldn't we check for contentType.startsWith("multipart/") ?
> 

Not sure - I think we're only supporting the x-mixed-replace MIME-type here. This is what WebCore itself looks for.

> >> WebCore/platform/network/qt/QNetworkReplyHandler.cpp:432
> >> +    int s = contentType.lastIndexOf("boundary=") + 9;
> >
> > "s" is not a good variable name.
> 
> the boundary could (and in fact, should) be quoted, recommended in RFC 2046 section 5.1.1; I think the patch should handle that use case as well
> 

Yes - thanks for the reference. There's quite a few cases to handle there!

> >> WebCore/platform/network/qt/QNetworkReplyHandler.cpp:446
> >> +        int bs = data->indexOf(boundary);
> >
> > "bs" is not a good variable name.
> 
> maybe combine the two above lines to
> 
> while ((boundaryIndex = data->indexOf(boundary)) != -1) {
> 
> the indexOf() method could be costly for big parts
> 
> > WebCore/platform/network/qt/QNetworkReplyHandler.cpp:450
> > +        data->replace(part, "");
> 
> how about keeping data constant and only use the char pointer and indexing? data might be a really big byte array, so detaching should be avoided. Or how about wrapping it into a QBuffer? Or could it somehow be avoided to construct the QByteArray in the first place, and operate on some char pointer instead?

Yes, that's much better. Works well.
Comment 8 Robert Hogan 2010-10-08 13:42:34 PDT
Created attachment 70282 [details]
Patch
Comment 9 Robert Hogan 2010-10-09 11:44:37 PDT
Created attachment 70358 [details]
Patch
Comment 10 Markus Goetz 2010-10-19 05:44:12 PDT
I'd say it might be the wrong layer to support this here and the support should be in QNetworkAccessManager. However there is a lack of people that can handle the stuff there, so I guess this is fine for now.

Which servers send multipart images and why? I have not heard of that before.

I think there should be comments in QNetworkReplyHandler::forwardData() and QNetworkReplyHandler::sendResponseIfNeeded() explaining why the function/data flow is going like this.

m_boundariesRead should be called m_multipartBoundariesRead IMHO
Comment 11 Robert Hogan 2010-10-19 11:41:04 PDT
(In reply to comment #10)
> I'd say it might be the wrong layer to support this here and the support should be in QNetworkAccessManager. However there is a lack of people that can handle the stuff there, so I guess this is fine for now.

Yeah, I'm of the same opinion now. Parsing data for HTTP headers just feels wrong in QNetworkReplyHandler. QNAM should feed it the individual sections of the multipart message with the appropriate header for each.

> 
> Which servers send multipart images and why? I have not heard of that before.
> 

http://en.wikipedia.org/wiki/Server_push#HTTP_server_push

It's a pretty old proposal/standard and ignored by IE. 

I think supporting this beyond streaming images, for which it was originally intended, may be a case of diminishing returns.

If the patch is too intrusive it may not be worth supporting at all.

I don't know who uses it in the wild.

> I think there should be comments in QNetworkReplyHandler::forwardData() and QNetworkReplyHandler::sendResponseIfNeeded() explaining why the function/data flow is going like this.
> 
> m_boundariesRead should be called m_multipartBoundariesRead IMHO

Will do.

Before I work on this further maybe we should decide how much of this (if any) we want to support?

A simple implementation that supports a multipart/x-mixed-replace response containing nothing but image sections requires the least overhead and will be achieved by this patch.

An implementation that supports a multipart response containing sections with different mime-types requires quite a bit more code.
Comment 12 John Brooks 2010-11-04 11:45:50 PDT
(In reply to comment #11)
> If the patch is too intrusive it may not be worth supporting at all.
> 
> I don't know who uses it in the wild.
> Before I work on this further maybe we should decide how much of this (if any) we want to support?
> 
> A simple implementation that supports a multipart/x-mixed-replace response containing nothing but image sections requires the least overhead and will be achieved by this patch.

MJPEG streams are implemented as a multipart/x-mixed-replace response with a new frame in each part. Supporting this case would be very useful - and most browsers do. Notably, many IP cameras use MJPEG streams.
Comment 13 Robert Hogan 2011-01-29 03:22:37 PST
So, do we want this? Should I clean it up for final review?
Comment 14 Robert Hogan 2011-02-08 12:12:57 PST
Comment on attachment 70358 [details]
Patch

Clearing review for now, going to come back to this.
Comment 15 Jocelyn Turcotte 2014-02-03 03:16:49 PST
=== Bulk closing of Qt bugs ===

If you believe that this bug report is still relevant for a non-Qt port of webkit.org, please re-open it and remove [Qt] from the summary.

If you believe that this is still an important QtWebKit bug, please fill a new report at https://bugreports.qt-project.org and add a link to this issue. See http://qt-project.org/wiki/ReportingBugsInQt for additional guidelines.