nsPNGEncoder and nsJPEGEncoder refuse to block

RESOLVED FIXED in mozilla2.0b1

Status

()

defect
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: khuey, Assigned: khuey)

Tracking

unspecified
mozilla2.0b1
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

nsPNGEncoder and nsJPEGEncoder both return false to IsNonBlocking, but in their implementations of ReadSegments, they both refuse to block.  They instead return a read length of 0 which is supposed to indicate EOF.  Consumers who are not aware of their odd behavior do not get the entire encoded image.  As a result, consumers end up doing stupid things like this http://mxr.mozilla.org/mozilla-central/source/content/html/content/src/nsHTMLCanvasElement.cpp#263 where Canvas spins a loop on reading the stream even though it's supposed to be a synchronous read.

I propose that we make imgIEncoder derive from nsIAsyncInputStream and do this properly.  We'd have to go fix up the consumers (there are 4 in the tree) that do silly stuff to make what is essentially an async stream behave synchronously.
(In reply to comment #0)
> not aware of their odd behavior do not get the entire encoded image.  As a
> result, consumers end up doing stupid things like this
> http://mxr.mozilla.org/mozilla-central/source/content/html/content/src/nsHTMLCanvasElement.cpp#263
> where Canvas spins a loop on reading the stream even though it's supposed to be
> a synchronous read.

Actually, that's what it should look like if it were in fact sync, so ignore that bit.  Everything else I said stands though.
Posted patch Make encoders asynchronous (obsolete) — Splinter Review
This patch makes imgIEncoder derive from nsIAsyncInputStream and implements the necessary methods on nsJPEGEncoder and nsPNGEncoder.  It also adds tests for the PNG encoder's implementation.  (It doesn't really make sense to test the JPEG encoder because it doesn't implement [Start|End]ImageEncode)  This doesn't affect the current consumers because in all of the existing cases the user fills the encoder with data and then immediately reads out the encoded image.  This patch makes it possible to hand out an initialized but empty imgIEncoder to anything expecting an nsIInputStream and then to later fill the imgIEncoder at your leisure.

This is needed for camera work we're doing for Fennec.
Attachment #447340 - Flags: review?
Attachment #447340 - Attachment is patch: true
Attachment #447340 - Attachment mime type: application/octet-stream → text/plain
Attachment #447340 - Flags: review? → review?(joe)
Comment on attachment 447340 [details] [diff] [review]
Make encoders asynchronous

>diff --git a/modules/libpr0n/encoders/jpeg/nsJPEGEncoder.cpp b/modules/libpr0n/encoders/jpeg/nsJPEGEncoder.cpp

>+NS_IMETHODIMP nsJPEGEncoder::AsyncWait(nsIInputStreamCallback *aCallback,
>+                                       PRUint32 aFlags,
>+                                       PRUint32 aRequestedCount,
>+                                       nsIEventTarget *aTarget)
>+{
>+  if (aFlags != 0)
>+    return NS_ERROR_NOT_IMPLEMENTED;
>+
>+  if (mCallback || mCallbackTarget)
>+    return NS_ERROR_UNEXPECTED;
>+  mCallback = aCallback;
>+  mCallbackTarget = aTarget;
>+  // 0 means "any number of bytes except 0"
>+  mNotifyThreshold = aRequestedCount;
>+  if (!aRequestedCount)
>+    mNotifyThreshold = 1;
>+
>+  // What we are being asked for may be present already
>+  NotifyListeners();
>+  return NS_OK;
>+}

Two comments about this:
 - NotifyListeners() should be called NotifyListener(), since we only have one
 - I'm pretty sure that NotifyListener() should never be called synchronously (reentrantly). Synchronous notification has caused us no end of trouble in the past. But I'll defer to Boris on this.

Totally fine otherwise. If we decide to make NotifyListener() from AsyncWait() asynchronous, we should add a test to ensure we never call it synchronously too.
Attachment #447340 - Flags: superreview?(bzbarsky)
Comment on attachment 447340 [details] [diff] [review]
Make encoders asynchronous

Tentative r-, dependent on the outcome of Boris' feedback.
Attachment #447340 - Flags: review?(joe) → review-
Comment on attachment 447340 [details] [diff] [review]
Make encoders asynchronous

This has a threading issue if the write callback calls NotifyListener while somebody is in the middle of AsyncWaiting on the stream.
Attachment #447340 - Attachment is obsolete: true
Attachment #447340 - Flags: superreview?(bzbarsky)
Posted patch Better Patch (obsolete) — Splinter Review
This is a better patch.  It fixes two threading issues, the first which allowed NotifyListener to be called simultaneously on different threads which would lead to either a truncated image if the second thread happened to be the correct thread or thread safety aborts if it did not.  The second allowed a buffer being read by a caller on one thread to be realloced away by a write callback on another.
Attachment #448659 - Flags: superreview?(bzbarsky)
Attachment #448659 - Flags: review?
Comment on attachment 448659 [details] [diff] [review]
Better Patch

A few comments:

0)  Please add -p to your diff options.
1)  Document what members are protected by mMonitor?
2)  In NotifyListener (both versions) there are overparenthesized conditions.
3)  If the aWriter passed to ReadSegments decides to call AsyncWait, you will
    deadlock.  Is that ok?  Can the monitor scopes be narrowed?
4)  If the caller of AsyncWait gets notified synchronously and immediately calls
    AsyncWait again (under its OnInputStreamReady), you will deadlock.  That's
    almost certainly not ok.
5)  If NotifyLister happens when mCallbackTarget is null, you will deadlock
    (because the whole point of calling OnInputStreamReady is for the callee to
    call ReadSegments, and then you lose).
Attachment #448659 - Flags: superreview?(bzbarsky) → superreview-
(In reply to comment #7)
> (From update of attachment 448659 [details] [diff] [review])
> A few comments:
> 
> 0)  Please add -p to your diff options.
> 1)  Document what members are protected by mMonitor?
> 2)  In NotifyListener (both versions) there are overparenthesized conditions.
> 3)  If the aWriter passed to ReadSegments decides to call AsyncWait, you will
>     deadlock.  Is that ok?  Can the monitor scopes be narrowed?
> 4)  If the caller of AsyncWait gets notified synchronously and immediately
> calls
>     AsyncWait again (under its OnInputStreamReady), you will deadlock.  That's
>     almost certainly not ok.
> 5)  If NotifyLister happens when mCallbackTarget is null, you will deadlock
>     (because the whole point of calling OnInputStreamReady is for the callee to
>     call ReadSegments, and then you lose).

As we discussed on IRC, 3, 4, 5 are not a concern because a Monitor can be reentered by the holding thread.  Will fix 0, 1, and 2.  Thanks for the review!
Comment on attachment 448659 [details] [diff] [review]
Better Patch

Clearing my review on this; please reflag me when you get an updated patch!
Attachment #448659 - Flags: review?(joe)
Posted patch PatchSplinter Review
Addressed bz's comments.
Attachment #448659 - Attachment is obsolete: true
Attachment #451067 - Flags: superreview?(bzbarsky)
Attachment #451067 - Flags: review?(joe)
Comment on attachment 451067 [details] [diff] [review]
Patch

sr=bzbarsky
Attachment #451067 - Flags: superreview?(bzbarsky) → superreview+
Comment on attachment 451067 [details] [diff] [review]
Patch

It sucks a little that these encoders need two separate implementations of the same interface. C'est la vie....


>diff --git a/modules/libpr0n/encoders/png/nsPNGEncoder.cpp b/modules/libpr0n/encoders/png/nsPNGEncoder.cpp

>+NS_IMETHODIMP nsPNGEncoder::AsyncWait(nsIInputStreamCallback *aCallback,
>+                                      PRUint32 aFlags,
>+                                      PRUint32 aRequestedCount,
>+                                      nsIEventTarget *aTarget)
>+  // 0 means "any number of bytes except 0"
>+  mNotifyThreshold = aRequestedCount;
>+  if (!aRequestedCount)
>+    mNotifyThreshold = 1;

In the JPEG encoder, you set this to 1k. Is there a reason to make it different here?
Attachment #451067 - Flags: review?(joe) → review+
(In reply to comment #12)
> In the JPEG encoder, you set this to 1k. Is there a reason to make it different
> here?

No, that was an oversight on my part.  Fixed in the push.

http://hg.mozilla.org/mozilla-central/rev/592ccf5c1a00
Status: NEW → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a6
Depends on: 575708
You need to log in before you can comment on or make changes to this bug.