Closed Bug 1072528 Opened 10 years ago Closed 10 years ago

Remove NPAPI async drawing model

Categories

(Core Graveyard :: Plug-ins, defect)

x86_64
Linux
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla36

People

(Reporter: roc, Assigned: roc)

References

Details

Attachments

(3 files, 3 obsolete files)

No-one's using it. No-one's going to use it.
You're talking about the async drawing that is exposed as part of NPAPI itself, and not the async drawing that we use internally for OOP plugins which draw synchronously within the plugin, right?
Flags: needinfo?(roc)
Correct.
Flags: needinfo?(roc)
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #0)
> No-one's using it.

Is this coming from telemetry data?
(In reply to Georg Fritzsche [:gfritzsche] from comment #5)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #0)
> > No-one's using it.
> 
> Is this coming from telemetry data?

We primarily designed it to be used by Flash. We know they're not using it. No-one has ever heard of anyone else using it. AFAIK no bugs have ever been reported regarding this feature. No other browser supports it. So I'm pretty confident.
Comment on attachment 8494757 [details] [diff] [review]
Part 1: Remove plugin-related code

the npapi/npfunction.h changes need to be upstream in https://code.google.com/p/npapi-sdk/ and we need to leave empty struct slots for them in _NPPluginFuncs. r- for that piece only, I think the rest is fine.

I don't know whether we ought to rev NP_VERSION_MINOR, I suspect we should.

This patch makes us return an error code if the plugin calls NPN_GetValue(NPNVsupportsAsyncBitmapSurfaceBool), when previously we would have set false and return NPERR_NO_ERROR. I don't expect that to be a problem, but is that what you intended?
Attachment #8494757 - Flags: review?(benjamin) → review-
Attachment #8494758 - Flags: review?(bas) → review+
Attached patch NPAPI patch (obsolete) — Splinter Review
Is this OK for NPAPI upstream?
Attachment #8497283 - Flags: review?(benjamin)
Comment on attachment 8497283 [details] [diff] [review]
NPAPI patch

We should "reserve" the removed values (7-8 for NPDrawingModel and 2007-2008 for NPNURLVariable. Maybe leave the enum values in but change the name to NPDrawingModelAsyncBitmapSurfaceOBSOLETE?

I think you'll also need to stub out NPN_InitAsyncSurfacePtr and mark things in _NPNetscapeFuncs as OBSOLETE (without removing them) for this to build correctly. NPN_InitAsyncSurfacePtr relies on the NPAsyncSurface struct.
Attachment #8497283 - Flags: review?(benjamin) → review-
Attached patch NPAPI patch v2 (obsolete) — Splinter Review
Is this better?

I assume the second part of your comment refers to Gecko code which isn't part of this patch.
Attachment #8497283 - Attachment is obsolete: true
Attachment #8497917 - Flags: review?(benjamin)
Comment on attachment 8497917 [details] [diff] [review]
NPAPI patch v2

npfunctions.h which is part of npapi-sdk, specifically https://code.google.com/p/npapi-sdk/source/browse/trunk/headers/npfunctions.h#71, #130-132, #156 and #217-219

The typedefs in npfunctions won't currently compile because the NPImageFormat and NPAsyncSurface types are gone, but we should just remove the typedefs and make the function pointers dummy struct placeholders.

This part of the change is ok, so I'm going to mark r+ on that.
Attachment #8497917 - Flags: review?(benjamin) → review+
Attached patch NPAPI patch v3Splinter Review
Ah OK. How about this then?
Attachment #8497917 - Attachment is obsolete: true
Attachment #8498504 - Flags: review?(benjamin)
Comment on attachment 8498504 [details] [diff] [review]
NPAPI patch v3

LGTM
Attachment #8498504 - Flags: review?(benjamin) → review+
Comment on attachment 8498504 [details] [diff] [review]
NPAPI patch v3

Review of attachment 8498504 [details] [diff] [review]:
-----------------------------------------------------------------

Josh, can you upstream this please?
Attachment #8498504 - Flags: review?(joshmoz)
Attachment #8498504 - Flags: review?(joshmoz) → review+
upstreamed
Attached patch Part 1 v2Splinter Review
Revised part 1, with the upstreamed NPAPI changes downstreamed.
Attachment #8494757 - Attachment is obsolete: true
Attachment #8504455 - Flags: review?(benjamin)
Comment on attachment 8504455 [details] [diff] [review]
Part 1 v2

I love reviewing patches like 32 insertions(+), 981 deletions(-)
Attachment #8504455 - Flags: review?(benjamin) → review+
https://hg.mozilla.org/mozilla-central/rev/f321139439a1
https://hg.mozilla.org/mozilla-central/rev/20d9430a0d62
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
I would like to disagree with the argument “No-one’s using it”:
Our (Cisco) product is deployed and used by major service providers (e.g. DIRECTV and more) who depend on this functionality until we move away from NPAPI (our plugin is one of Firefox whitelisted plugins – https://bugzilla.mozilla.org/show_bug.cgi?id=981403).
We have spent time and effort to support this mode in order to overcome the performance issues caused by using windowless plugin – issues which can’t be addressed in any other way.
Many other plugin developers use this functionality through the Firebreath plugin framework (http://www.firebreath.org/display/documentation/Windows+Asynchronous+Surface+Rendering).
Please reconsider the decision to remove this feature.
That's good to know! I'm stunned that we don't see any bugs being reported on this feature though.

bsmedberg, what do you think?
Flags: needinfo?(benjamin)
rohaber, can you tell us what your plugin actually is and does?
Flags: needinfo?(rohaber)
Also, is there a way we can try it out? If there's a chance we can make the existing windowless drawing code fast enough for you, we should take it.
Can we please deploy telemetry to judge the actual impact here?
(In reply to Georg Fritzsche [:gfritzsche] from comment #24)
> Can we please deploy telemetry to judge the actual impact here?
(and hold this off until we know more)
Does the plugin fall back to normal painting properly? If not we should clearly re-add this at least for a few releases.

If it does fall back properly, I'm less inclined to add this back, especially since the code is only going to be exercized by automated tests and this minority plugin. I don't think that telemetry is going to provide an especially compelling signal.
Flags: needinfo?(benjamin)
I'm not sure if low noise on bugs on a feature is a good signal for NPAPI either.
Do we have any other sources to base this on?
The plugin description (from the whitelisting bug I mentioned above):
The VGConnect browser plugin is a secure player used to protect premium video content.
The plugin is customized per video provider (e.g. on DIRECTV.com it is called “DIRECTV Player”).

Regarding a way to use the plugin, you can find installation instruction on the whitelisting bug page.

The problem with windowless drawing is we have to stretch our video frames *before* we are blitting them into the browser supplied HDC - which means we have to copy a very large memory block (i.e. 1920*1080*4 in case plugin is in HTML5 fullscreen mode) in very short time (60 fps). Async drawing is the only way to do so currently (which works also on low-mid range machines). Not sure it is something you can fix without changing the NPAPI interface.

Regarding the plugin distribution - as a security product provider, we cannot always disclose all the service providers that use our technology. Two examples that we can share with you are Charter & DIRECTV - we will be able to share more service providers that use our plugin soon.
Flags: needinfo?(rohaber)
(In reply to Ronen Haber (Cisco) from comment #28)
> The problem with windowless drawing is we have to stretch our video frames
> *before* we are blitting them into the browser supplied HDC - which means we
> have to copy a very large memory block (i.e. 1920*1080*4 in case plugin is
> in HTML5 fullscreen mode) in very short time (60 fps). Async drawing is the
> only way to do so currently (which works also on low-mid range machines).
> Not sure it is something you can fix without changing the NPAPI interface.

Seems to me you could have the plugin draw unscaled and apply a CSS transform to the plugin to do the scaling. Then we'd be able to scale on the GPU or at least on the compositor thread. Would that work?
Flags: needinfo?(rohaber)
Also, what do you do for Chrome?
Also I'm curious why you can't use windowed mode.
CSS transform might work in some cases (can you point me to a relevant example?) - but it is impossible to use when we need to draw subtitles/Closed Captions also (as we must draw them on the final size frame - otherwise we will get blurred text).

Chrome is indeed a problem - and our clients knows that if they want to use HTML5 based fullscreen (instead of our native plugin fullscreen), they will suffer from performance degradation (I've also opened a bug 2 years ago asking them to implement those models: https://code.google.com/p/chromium/issues/detail?id=130520).

Windowed mode is not an option since our customers are drawing overlays (player controls, etc.) on top of our plugin. (And year ago, wiki page https://wiki.mozilla.org/Plugins:Goals claimed Mozilla is going to remove the Windowed mode...)
Flags: needinfo?(rohaber)
(In reply to Ronen Haber (Cisco) from comment #32)
> CSS transform might work in some cases (can you point me to a relevant
> example?)

You could use "object { transform-origin:top left; transform:scale(<scale>); }" to stretch your plugin horizontally and vertically.

> - but it is impossible to use when we need to draw
> subtitles/Closed Captions also (as we must draw them on the final size frame
> - otherwise we will get blurred text).

Blurred text is suboptimal but not the end of the world. After all, it's common to see videos with burned-in subtitles that get blurred the same way.

I don't think we can justify keeping the NPAPI async drawing model API --- which has architectural consequences fairly deep into our graphics stack, which are impacting important projects like multiprocess browsing --- to avoid blurred caption text in your plugin. Sorry.

> Chrome is indeed a problem - and our clients knows that if they want to use
> HTML5 based fullscreen (instead of our native plugin fullscreen), they will
> suffer from performance degradation (I've also opened a bug 2 years ago
> asking them to implement those models:
> https://code.google.com/p/chromium/issues/detail?id=130520).

If removing the async drawing model leaves us with performance no worse than Chrome, then I don't think we can consider this a blocking bug.

> Windowed mode is not an option since our customers are drawing overlays
> (player controls, etc.) on top of our plugin. (And year ago, wiki page
> https://wiki.mozilla.org/Plugins:Goals claimed Mozilla is going to remove
> the Windowed mode...)

OK.
Actually, we are also using these rendering modes. I will disclose further details after consulting my manager, to avoid to possibility of leaking potentially confidential information.
(In reply to Milan Burda (Skype/Microsoft) from comment #34)
> Actually, we are also using these rendering modes. I will disclose further
> details after consulting my manager, to avoid to possibility of leaking
> potentially confidential information.

... the possibility ...
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #6)
> (In reply to Georg Fritzsche [:gfritzsche] from comment #5)
> > (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #0)
> > > No-one's using it.
> > 
> > Is this coming from telemetry data?
> 
> We primarily designed it to be used by Flash. We know they're not using it.
> No-one has ever heard of anyone else using it. AFAIK no bugs have ever been
> reported regarding this feature. No other browser supports it. So I'm pretty
> confident.

Hi Robert, I'm developing a plugin with firebreath, and I need to use the "Windows Asynchronous Surface Rendering" feature. After testing, I'm sure the IE(version >=9) support this feature. But you removed this feature since version 36 in firefox. Does this feature could be opened in latest version by configure anything? This feature is very important for me!
Flags: needinfo?(xinjiguaike)
Flags: needinfo?(xinjiguaike)
#Comment36#
Flags: needinfo?(roc)
No, this has been removed from the code and we're not putting it back, sorry.
Flags: needinfo?(roc)
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.