Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
9331003:
Improving the decoder pipeline. (Closed)
|
Created:
8 years, 10 months ago by alexeypa (please no reviews) Modified:
8 years, 10 months ago Reviewers:
Wez CC:
Jamie, chromium-reviews, jamiewalch+watch_chromium.org, dcaiafa+watch_chromium.org, simonmorris+watch_chromium.org, hclam+watch_chromium.org, wez+watch_chromium.org, amit, sanjeevr, garykac+watch_chromium.org, lambroslambrou+watch_chromium.org, alexeypa+watch_chromium.org, sergeyu+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionThis CL makes several the following improvements to the Chromoting decoder pipeline:
1. Only the clipping area, not the full frame, is drawn. This reduces the risk of out of memory situation on high page zoom levels. Screen updates are also incremental including scrolling scenarios.
2. Decoders now write pixels directly to the Pepper API backing store making it one memcpy less.
3. Scaling and panning is fully controlled by the plugin. The decoder only supplies the pixels it was asked for by the plugin.
BUG=109938
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=123573
Patch Set 1 #
Total comments: 38
Patch Set 2 : Redesigned FrameConsumer/FrameProducer. #
Total comments: 10
Patch Set 3 : CR feedback #Patch Set 4 : rebased #
Total comments: 108
Patch Set 5 : CR feedback and a couple of fixes. #
Total comments: 1
Patch Set 6 : CR feedback. #
Total comments: 6
Patch Set 7 : More CR feedback. #
Total comments: 6
Patch Set 8 : A rebased fixup. #Patch Set 9 : Integer ScaleRect #
Messages
Total messages: 22 (0 generated)
Please take a look as the decoder pipeline changes. This CL is going to be updated once the pending changes will make it through the CQ.
Comments ahoy. Could you re-base after you've landed the CLs that are mixed in with this one? https://chromiumcodereview.appspot.com/9331003/diff/1/remoting/base/codec_tes... File remoting/base/codec_test.cc (right): https://chromiumcodereview.appspot.com/9331003/diff/1/remoting/base/codec_tes... remoting/base/codec_test.cc:12: #include "ppapi/cpp/image_data.h" Is this needed? https://chromiumcodereview.appspot.com/9331003/diff/1/remoting/base/compresso... File remoting/base/compressor_verbatim.cc (right): https://chromiumcodereview.appspot.com/9331003/diff/1/remoting/base/compresso... remoting/base/compressor_verbatim.cc:29: return (flush != CompressorFinish) || (output_size < bytes_to_copy); This looks like a fix to the CompressorVerbatim? https://chromiumcodereview.appspot.com/9331003/diff/1/remoting/base/decoder.h File remoting/base/decoder.h (right): https://chromiumcodereview.appspot.com/9331003/diff/1/remoting/base/decoder.h... remoting/base/decoder.h:37: // Initializes the decoder and sets the output dimensions for the decoder. nit: no need for "for the decoder". https://chromiumcodereview.appspot.com/9331003/diff/1/remoting/base/decoder.h... remoting/base/decoder.h:38: virtual void Initialize(const SkISize& screen_size) = 0; Why do you need |screen_size| here? https://chromiumcodereview.appspot.com/9331003/diff/1/remoting/base/decoder.h... remoting/base/decoder.h:55: virtual void UpdateRegion(const SkRegion& region) = 0; This should be called ForceUpdate(), Invalidate(), or something along those lines, to make its function clearer. https://chromiumcodereview.appspot.com/9331003/diff/1/remoting/base/decoder.h... remoting/base/decoder.h:58: // performing necessary conversion and scaling. The routine actually refreshes the portions of the video frame within the specified |clip_area|, that have changed since the last frame, or explicitly marked to be refreshed? https://chromiumcodereview.appspot.com/9331003/diff/1/remoting/base/decoder.h... remoting/base/decoder.h:60: // The routine produces |output_region| to indicate the updated areas of produces ... -> sets ... https://chromiumcodereview.appspot.com/9331003/diff/1/remoting/base/decoder.h... remoting/base/decoder.h:64: // (RGBA32). The top left corner of the buffer corresponds to the top left What coordinate scheme does |clip_area| apply to? Input or output? https://chromiumcodereview.appspot.com/9331003/diff/1/remoting/base/decoder.h... remoting/base/decoder.h:67: virtual void Draw(const SkISize& view_size, I think it's cleaner to set the output size explicitly, via a separate call, then to pass it here. https://chromiumcodereview.appspot.com/9331003/diff/1/remoting/base/decoder.h... remoting/base/decoder.h:71: SkRegion* output_region) = 0; Consider renaming this RefreshFrame(), UpdateFrame(), RenderFrame(), or similar. https://chromiumcodereview.appspot.com/9331003/diff/1/remoting/base/decoder_v... File remoting/base/decoder_vp8.cc (right): https://chromiumcodereview.appspot.com/9331003/diff/1/remoting/base/decoder_v... remoting/base/decoder_vp8.cc:103: state_ = kUninitialized; Do we need this, since when we use it, we immediately re-initialize? https://chromiumcodereview.appspot.com/9331003/diff/1/remoting/base/decoder_v... File remoting/base/decoder_vp8.h (right): https://chromiumcodereview.appspot.com/9331003/diff/1/remoting/base/decoder_v... remoting/base/decoder_vp8.h:49: SkRegion updated_region_; I think this needs renaming, and the semantics clarifying. This region gets populated either from an incoming update, or an UpdateRegion call, and cleared after each Draw(). But what if we Draw only a portion of the desktop, but are keeping a full-sized framebuffer, i.e. we're just using clipping to optimize out some work? Then we've actually lost information on what changed in those other regions. https://chromiumcodereview.appspot.com/9331003/diff/1/remoting/base/util.h File remoting/base/util.h (right): https://chromiumcodereview.appspot.com/9331003/diff/1/remoting/base/util.h#ne... remoting/base/util.h:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. This changes in this and util.cc look to be ones I've reviewd elsewhere, so I'm skipping them. https://chromiumcodereview.appspot.com/9331003/diff/1/remoting/client/frame_c... File remoting/client/frame_consumer.h (right): https://chromiumcodereview.appspot.com/9331003/diff/1/remoting/client/frame_c... remoting/client/frame_consumer.h:27: // the backing store where the updated pixels should be written to. Not sure what it means for it to "capture" them? https://chromiumcodereview.appspot.com/9331003/diff/1/remoting/client/frame_c... remoting/client/frame_consumer.h:27: // the backing store where the updated pixels should be written to. It looks like OnFrameReady() is used to indicate that there is data ready to render, and it asynchronously determines the current clip area, view dimensions and backing-store and returns them. Won't that introduce extra latency in the common case where none of those properties are changing? https://chromiumcodereview.appspot.com/9331003/diff/1/remoting/client/frame_c... remoting/client/frame_consumer.h:31: scoped_ptr<pp::ImageData>* backing_store_out, Rather than having out-parameters, define a frame-ready callback to replace |done|. https://chromiumcodereview.appspot.com/9331003/diff/1/remoting/client/frame_c... remoting/client/frame_consumer.h:37: scoped_ptr<SkRegion> updated_region) = 0; No completion indicator? How do we rate-limit to match the system's rendering throughput? https://chromiumcodereview.appspot.com/9331003/diff/1/remoting/client/rectang... File remoting/client/rectangle_update_decoder.cc (right): https://chromiumcodereview.appspot.com/9331003/diff/1/remoting/client/rectang... remoting/client/rectangle_update_decoder.cc:72: decoder_needs_reset = true; So we really need to reset the decoder on host resize? The old implementation passed the decoder a host-sized frame to output into, which necessitated a "reset". https://chromiumcodereview.appspot.com/9331003/diff/1/remoting/client/rectang... File remoting/client/rectangle_update_decoder.h (right): https://chromiumcodereview.appspot.com/9331003/diff/1/remoting/client/rectang... remoting/client/rectangle_update_decoder.h:43: // registered as data is avaialable. DecodePacket may keep a reference to This class doesn't have an OnFrameReady, though? typo: available https://chromiumcodereview.appspot.com/9331003/diff/1/remoting/client/rectang... remoting/client/rectangle_update_decoder.h:45: // executed. nit: If we pass |packet| as a scoped_ptr<> then we don't need this scary-sounding comment. https://chromiumcodereview.appspot.com/9331003/diff/1/remoting/protocol/conte... File remoting/protocol/content_description.cc (right): https://chromiumcodereview.appspot.com/9331003/diff/1/remoting/protocol/conte... remoting/protocol/content_description.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. These look like changes from an earlier CL? https://chromiumcodereview.appspot.com/9331003/diff/1/remoting/webapp/client_... File remoting/webapp/client_session.js (right): https://chromiumcodereview.appspot.com/9331003/diff/1/remoting/webapp/client_... remoting/webapp/client_session.js:446: var scale = Math.round(100.0 * viewInnerWidth / viewOuterWidth) / 100.0; This looks like a change from a different CL, so I'm skipping it.
Could you please take a look? (It is large. Sorry). http://codereview.chromium.org/9331003/diff/1/remoting/base/codec_test.cc File remoting/base/codec_test.cc (right): http://codereview.chromium.org/9331003/diff/1/remoting/base/codec_test.cc#new... remoting/base/codec_test.cc:12: #include "ppapi/cpp/image_data.h" On 2012/02/07 01:56:31, Wez wrote: > Is this needed? No. Removed. http://codereview.chromium.org/9331003/diff/1/remoting/base/compressor_verbat... File remoting/base/compressor_verbatim.cc (right): http://codereview.chromium.org/9331003/diff/1/remoting/base/compressor_verbat... remoting/base/compressor_verbatim.cc:29: return (flush != CompressorFinish) || (output_size < bytes_to_copy); On 2012/02/07 01:56:31, Wez wrote: > This looks like a fix to the CompressorVerbatim? Yes. It is checked in already. http://codereview.chromium.org/9331003/diff/1/remoting/base/decoder.h File remoting/base/decoder.h (right): http://codereview.chromium.org/9331003/diff/1/remoting/base/decoder.h#newcode37 remoting/base/decoder.h:37: // Initializes the decoder and sets the output dimensions for the decoder. On 2012/02/07 01:56:31, Wez wrote: > nit: no need for "for the decoder". Done. http://codereview.chromium.org/9331003/diff/1/remoting/base/decoder.h#newcode38 remoting/base/decoder.h:38: virtual void Initialize(const SkISize& screen_size) = 0; On 2012/02/07 01:56:31, Wez wrote: > Why do you need |screen_size| here? For scaling. It is passed this way because RectangeUpdateDecoder is responsible for extracting frame dimensions from a packet. http://codereview.chromium.org/9331003/diff/1/remoting/base/decoder.h#newcode55 remoting/base/decoder.h:55: virtual void UpdateRegion(const SkRegion& region) = 0; On 2012/02/07 01:56:31, Wez wrote: > This should be called ForceUpdate(), Invalidate(), or something along those > lines, to make its function clearer. It is Invalidate() now. http://codereview.chromium.org/9331003/diff/1/remoting/base/decoder.h#newcode58 remoting/base/decoder.h:58: // performing necessary conversion and scaling. On 2012/02/07 01:56:31, Wez wrote: > The routine actually refreshes the portions of the video frame within the > specified |clip_area|, that have changed since the last frame, or explicitly > marked to be refreshed? Both. http://codereview.chromium.org/9331003/diff/1/remoting/base/decoder.h#newcode60 remoting/base/decoder.h:60: // The routine produces |output_region| to indicate the updated areas of On 2012/02/07 01:56:31, Wez wrote: > produces ... -> sets ... Done. http://codereview.chromium.org/9331003/diff/1/remoting/base/decoder.h#newcode64 remoting/base/decoder.h:64: // (RGBA32). The top left corner of the buffer corresponds to the top left On 2012/02/07 01:56:31, Wez wrote: > What coordinate scheme does |clip_area| apply to? Input or output? Consumer's. Done. http://codereview.chromium.org/9331003/diff/1/remoting/base/decoder.h#newcode67 remoting/base/decoder.h:67: virtual void Draw(const SkISize& view_size, On 2012/02/07 01:56:31, Wez wrote: > I think it's cleaner to set the output size explicitly, via a separate call, > then to pass it here. This call does not set the output size. |view_size| is used to see if scaling should be done. http://codereview.chromium.org/9331003/diff/1/remoting/base/decoder.h#newcode71 remoting/base/decoder.h:71: SkRegion* output_region) = 0; On 2012/02/07 01:56:31, Wez wrote: > Consider renaming this RefreshFrame(), UpdateFrame(), RenderFrame(), or similar. RenderFrame(). Done. http://codereview.chromium.org/9331003/diff/1/remoting/base/decoder_vp8.cc File remoting/base/decoder_vp8.cc (right): http://codereview.chromium.org/9331003/diff/1/remoting/base/decoder_vp8.cc#ne... remoting/base/decoder_vp8.cc:103: state_ = kUninitialized; On 2012/02/07 01:56:31, Wez wrote: > Do we need this, since when we use it, we immediately re-initialize? Done. http://codereview.chromium.org/9331003/diff/1/remoting/base/decoder_vp8.h File remoting/base/decoder_vp8.h (right): http://codereview.chromium.org/9331003/diff/1/remoting/base/decoder_vp8.h#new... remoting/base/decoder_vp8.h:49: SkRegion updated_region_; On 2012/02/07 01:56:31, Wez wrote: > I think this needs renaming, and the semantics clarifying. This region gets > populated either from an incoming update, or an UpdateRegion call, and cleared > after each Draw(). > > But what if we Draw only a portion of the desktop, but are keeping a full-sized > framebuffer, i.e. we're just using clipping to optimize out some work? Then > we've actually lost information on what changed in those other regions. Now we validate only the painted area. http://codereview.chromium.org/9331003/diff/1/remoting/client/frame_consumer.h File remoting/client/frame_consumer.h (right): http://codereview.chromium.org/9331003/diff/1/remoting/client/frame_consumer.... remoting/client/frame_consumer.h:27: // the backing store where the updated pixels should be written to. On 2012/02/07 01:56:31, Wez wrote: > Not sure what it means for it to "capture" them? This interface was completely changed. So this comment and the comments below are not not relevant. http://codereview.chromium.org/9331003/diff/1/remoting/client/rectangle_updat... File remoting/client/rectangle_update_decoder.cc (right): http://codereview.chromium.org/9331003/diff/1/remoting/client/rectangle_updat... remoting/client/rectangle_update_decoder.cc:72: decoder_needs_reset = true; On 2012/02/07 01:56:31, Wez wrote: > So we really need to reset the decoder on host resize? The old implementation > passed the decoder a host-sized frame to output into, which necessitated a > "reset". I don't know but I'd like to avoid adding more changes to this change list. http://codereview.chromium.org/9331003/diff/1/remoting/client/rectangle_updat... File remoting/client/rectangle_update_decoder.h (right): http://codereview.chromium.org/9331003/diff/1/remoting/client/rectangle_updat... remoting/client/rectangle_update_decoder.h:43: // registered as data is avaialable. DecodePacket may keep a reference to On 2012/02/07 01:56:31, Wez wrote: > This class doesn't have an OnFrameReady, though? > > typo: available Done. http://codereview.chromium.org/9331003/diff/1/remoting/client/rectangle_updat... remoting/client/rectangle_update_decoder.h:45: // executed. On 2012/02/07 01:56:31, Wez wrote: > nit: If we pass |packet| as a scoped_ptr<> then we don't need this > scary-sounding comment. I'd postpone this change till later. This change in large enough already.
Some comments on the JS for starters. http://codereview.chromium.org/9331003/diff/10001/remoting/webapp/client_sess... File remoting/webapp/client_session.js (right): http://codereview.chromium.org/9331003/diff/10001/remoting/webapp/client_sess... remoting/webapp/client_session.js:441: // be roughly estimated as a ratio between outerWidht and innerWidth of s/outerWidht/outerWidth/ http://codereview.chromium.org/9331003/diff/10001/remoting/webapp/client_sess... remoting/webapp/client_session.js:444: var viewInnerWidth = document.defaultView.innerWidth; Why document.defaultView instead of window? http://codereview.chromium.org/9331003/diff/10001/remoting/webapp/client_sess... remoting/webapp/client_session.js:446: var scale = Math.round(100.0 * viewOuterWidth / viewInnerWidth) / 100.0; Why convert to int so early? You can stick with floats throughout--this isn't performance critical. http://codereview.chromium.org/9331003/diff/10001/remoting/webapp/client_sess... remoting/webapp/client_session.js:448: // TODO(alexeypa): remove this hack once proper zooming API is available. Can you raise a ticket for this and include the number here, or reference an existing ticket (109933 perhaps?) http://codereview.chromium.org/9331003/diff/10001/remoting/webapp/client_sess... remoting/webapp/client_session.js:454: while (left < right) { Binary search seems overkill. In fact, you can simplify this a lot if you assume that outerWidth is at least innerWidth*zoom (you can't have scrollbars with negative width). In essence, you want the value of |zoom| that minimzes |chrome| in the equation: chrome = outer - inner*zoom http://codereview.chromium.org/9331003/diff/10001/remoting/webapp/client_sess... remoting/webapp/client_session.js:456: remoting.debug.log('middle=' + middle); Remove this debug?
Updated. http://codereview.chromium.org/9331003/diff/10001/remoting/webapp/client_sess... File remoting/webapp/client_session.js (right): http://codereview.chromium.org/9331003/diff/10001/remoting/webapp/client_sess... remoting/webapp/client_session.js:441: // be roughly estimated as a ratio between outerWidht and innerWidth of On 2012/02/16 00:26:32, Jamie wrote: > s/outerWidht/outerWidth/ Done. http://codereview.chromium.org/9331003/diff/10001/remoting/webapp/client_sess... remoting/webapp/client_session.js:444: var viewInnerWidth = document.defaultView.innerWidth; On 2012/02/16 00:26:32, Jamie wrote: > Why document.defaultView instead of window? Done. http://codereview.chromium.org/9331003/diff/10001/remoting/webapp/client_sess... remoting/webapp/client_session.js:446: var scale = Math.round(100.0 * viewOuterWidth / viewInnerWidth) / 100.0; On 2012/02/16 00:26:32, Jamie wrote: > Why convert to int so early? You can stick with floats throughout--this isn't > performance critical. Done. http://codereview.chromium.org/9331003/diff/10001/remoting/webapp/client_sess... remoting/webapp/client_session.js:456: remoting.debug.log('middle=' + middle); On 2012/02/16 00:26:32, Jamie wrote: > Remove this debug? Done.
FYI: rebased on top the current HEAD. Wez, could you please take a look? Jamie was looking at the JavaScript part yesterday, but everything else is yours now. :-)
https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/webapp/cli... File remoting/webapp/client_session.js (right): https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/webapp/cli... remoting/webapp/client_session.js:461: var delta = Math.abs(windowOuterWidth - windowWidth * zoom_levels[i]); I'm don't think that taking an absolute value is right. Given a choice of decoration widths of 16px or -15px, the former is far more likely, but this code will pick the latter. It seems we should reject zoom levels that suggest a negative window border, or am I missing something?
Added the last couple of fixes + CR feedback. https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/webapp/cli... File remoting/webapp/client_session.js (right): https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/webapp/cli... remoting/webapp/client_session.js:461: var delta = Math.abs(windowOuterWidth - windowWidth * zoom_levels[i]); On 2012/02/16 18:45:22, Jamie wrote: > I'm don't think that taking an absolute value is right. Given a choice of > decoration widths of 16px or -15px, the former is far more likely, but this code > will pick the latter. It seems we should reject zoom levels that suggest a > negative window border, or am I missing something? I added a comment explaining why it is needed. I was surprised too. :-)
Javascript looks good apart from a typo, but I'll let Wez sign off on the rest. https://chromiumcodereview.appspot.com/9331003/diff/17040/remoting/webapp/cli... File remoting/webapp/client_session.js (right): https://chromiumcodereview.appspot.com/9331003/diff/17040/remoting/webapp/cli... remoting/webapp/client_session.js:461: // The difference between outerWidht and scaled innerWidth should always be Nit: Typo
https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/base/decod... File remoting/base/decoder.h (right): https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/base/decod... remoting/base/decoder.h:50: // coordinates. nit: Force -> Forces https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/base/decod... remoting/base/decoder.h:50: // coordinates. Does this actually cause a frame to be output, or just force the Decoder to include the specified |region| the next time RenderFrame() is called? https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/base/decod... remoting/base/decoder.h:56: // to be invalidated. Only the pixels within |clip_area| are copied. nit: And pixels outside |clip_area| remain invalidated? https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/base/decod... File remoting/base/decoder_row_based.cc (right): https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/base/decod... remoting/base/decoder_row_based.cc:66: if (!screen_size_.isZero()) { isZero() -> isEmpty() https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/base/decod... remoting/base/decoder_row_based.cc:81: // The input buffer is a linear vector of pixel data. nit: This comment doesn't really add anything to the following lines https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/base/decod... remoting/base/decoder_row_based.cc:86: // Access the pixel buffer directly. nit: This comment doesn't really add anything https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/base/decod... remoting/base/decoder_row_based.cc:152: LOG(WARNING) << "Invalid clipping area received."; If this is never valid then consider a DCHECK, or NOTREACHED https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/base/decod... remoting/base/decoder_row_based.cc:205: // not match |screen_size_|. nit: Consider changing this to a comment bemoaning the lack of scaling, rather than a statement of intent to implement it. Or add a feature-tracking bug... ;) https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/base/decod... File remoting/base/decoder_vp8.cc (right): https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/base/decod... remoting/base/decoder_vp8.cc:149: updated_region_.op(ScaleRect(clip_area, view_size, screen_size_), This isn't strictly correct, I don't think. It'll cause pixels that *affect* the clip-area, but which also affect pixels outside the clip-area in the scaled view, not to be updated, leading to visual artefacts. https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/base/util.cc File remoting/base/util.cc (right): https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/base/util.... remoting/base/util.cc:222: // Use floating point to up/down scale more accurately. Given that the results are still integer, how is this more accurate than the existing code? https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/client/fra... File remoting/client/frame_consumer.h (right): https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/client/fra... remoting/client/frame_consumer.h:26: // not to the clipping area. nit: nor to the |buffer| coordinates https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/client/fra... remoting/client/frame_consumer.h:26: // not to the clipping area. Can you rephrase this comment to be more in keeping with other Chromium comments, e.g: // Accepts a buffer and update region to be painted to the screen. // ... void PaintBuffer() https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/client/fra... remoting/client/frame_consumer.h:28: const SkIRect& clip_area, If the top-left of |buffer| is assumed to match |clip_area| then |clip_area|'s width & height are effectively duplicating the |buffer| dimensions - perhaps it's cleaner to send just the |clip_area| origin instead? https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/client/fra... remoting/client/frame_consumer.h:29: pp::ImageData* buffer, Hmmm. We really want a PP_Resource equivalent of scoped_ptr<pp::ImageData>. Really, to keep this interface abstract, we should not use pp::IMageData here, but keep that an implementation detail of PepperView. https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/client/fra... remoting/client/frame_consumer.h:35: virtual void ReturnBuffer(pp::ImageData* buffer) = 0; scoped_ptr<>? https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/client/fra... remoting/client/frame_consumer.h:38: virtual void SetScreenSize(const SkISize& screen_size) = 0; This is the only API with a Chromoting-specific name, but it's function isn't actually Chromoting-specific - it just informs the FrameConsumer of the original dimensions of the media. How about SetSourceSize(), SetOriginalSize() or SetNaturalSize()? https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/client/fra... File remoting/client/frame_consumer_proxy.cc (right): https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/client/fra... remoting/client/frame_consumer_proxy.cc:67: frame_consumer_ = NULL; WHat happens to buffers in-flight when Detach() is called? https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/client/fra... File remoting/client/frame_producer.h (right): https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/client/fra... remoting/client/frame_producer.h:25: // the consumer. nit: "Request that all pending buffers be returned to the consumer via ReturnBuffer"? Consider renaming this to ReturnBuffers() or RequestReturnBuffers(). https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/client/fra... remoting/client/frame_producer.h:33: // The passed buffer must be large enough to hold the whole clipping area. I think this method needs a better name, and description! The method is used to add output buffers for the producer to use. Although there is a queue in the implementation, it's not strictly necessary for the producer to use a queue, I don't think. How about AddOutputBuffer()? https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/client/fra... remoting/client/frame_producer.h:39: // the frame. nit: Comment style, e.g: "Requests that the specified region of the frame be refreshed as soon as possible" https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/client/fra... remoting/client/frame_producer.h:40: virtual void Invalidate(const SkRegion& region) = 0; Consider calling this RequestRegion() or InvalidateRegion(). https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/client/fra... remoting/client/frame_producer.h:44: // all enqueued buffers to the consumer for reuse or reallocation. nit: Comment style, e.g: "Notifies the producer of changes to the output view size or visible (clip) area." https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/client/fra... remoting/client/frame_producer.h:45: virtual void SetView(const SkISize& view_size, const SkIRect& clip_area) = 0; "View" is a Pepper term; in abstract terms this sets the output size, and visible/clip area. How about SetOutputSizeAndClip()? https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/client/plu... File remoting/client/plugin/pepper_view.cc (right): https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/client/plu... remoting/client/plugin/pepper_view.cc:84: // The producer should now return any pending buffers. This comment doesn't seem to describe the line that follows it? https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/client/plu... remoting/client/plugin/pepper_view.cc:93: done_event.Wait(); Hmmm. It's a real shame to have to block here... https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/client/plu... remoting/client/plugin/pepper_view.cc:106: if (producer_disabled_ && !clip_area_.isEmpty()) { We should just strip out the static-fill code, I think... https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/client/plu... remoting/client/plugin/pepper_view.cc:189: bool updated = false; Can you give this a more helpful name, e.g. size_or_clip_changed? https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/client/plu... remoting/client/plugin/pepper_view.cc:191: // Prevent accidental upscaling. nit: It's more that we don't currently support up-scaling. https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/client/plu... remoting/client/plugin/pepper_view.cc:231: // The data in the buffer is useless if the scale factor has changed. Not strictly true; we could render what we have and thereby lag behind less noticably in the case of a gradual resize operation? https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/client/plu... remoting/client/plugin/pepper_view.cc:268: // Submit an update of desktop size to Javascript. nit: Suggest "Notify JavaScript of the change in size" https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/client/plu... remoting/client/plugin/pepper_view.cc:282: LOG(WARNING) << "Not enough memory for backing store."; nit: "backing store" -> "frame buffers" https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/client/plu... remoting/client/plugin/pepper_view.cc:325: // merge all pending changes into a single buffer and then apply it to nit: Suggest "this code should apply all pending changes to the screen" https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/client/plu... remoting/client/plugin/pepper_view.cc:336: base::Time start_time = base::Time::Now(); Gahhh. We should update these stats gathering points to use TimeTicks(), really. https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/client/plu... remoting/client/plugin/pepper_view.cc:344: // to be painted. How does this comment relate to the intersect() call below? https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/client/plu... remoting/client/plugin/pepper_view.cc:349: rect.offset(- clip_area.left(), - clip_area.top()); nit: Not 100% sure, but I think the '-' should not be separated from the terms, since they're the unary form. https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/client/plu... remoting/client/plugin/pepper_view.cc:352: // point here is the offset from the source rect. Why? It is within our power to fix the Graphics2D comments. ;) https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/client/plu... remoting/client/plugin/pepper_view.cc:359: // Flush the updated areas ro the screen. typo: ro -> to https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/client/plu... remoting/client/plugin/pepper_view.cc:371: // result value. This is ugly. We should be able to clean this up reasonably easily, I think? https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/client/plu... remoting/client/plugin/pepper_view.cc:380: ignore_result(task.release()); This doesn't look right. Shouldn't we be release()ing task for CompletionCallbackClosureAdapter to destroy? As it stands this code looks like it'll leak the task. https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/client/plu... File remoting/client/plugin/pepper_view.h (right): https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/client/plu... remoting/client/plugin/pepper_view.h:117: // True if there is pending paint commands in Pepper's queue. This is set to nit: is -> are Consider rewording: True if there is already a Flush() pending on the Graphics2D context. https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/client/plu... remoting/client/plugin/pepper_view.h:121: // When true no new work should be posted to the producer. Doesn't work come from the RectangleUpdateDecoder? https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/client/rec... File remoting/client/rectangle_update_decoder.cc (right): https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/client/rec... remoting/client/rectangle_update_decoder.cc:171: // updated because they haven't been validated by RenderFrame() yet. This sentence sounds the wrong way around?
PTAL https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/base/decod... File remoting/base/decoder.h (right): https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/base/decod... remoting/base/decoder.h:50: // coordinates. On 2012/02/17 23:42:17, Wez wrote: > nit: Force -> Forces Both comments: Done. https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/base/decod... remoting/base/decoder.h:56: // to be invalidated. Only the pixels within |clip_area| are copied. On 2012/02/17 23:42:17, Wez wrote: > nit: And pixels outside |clip_area| remain invalidated? Done. https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/base/decod... File remoting/base/decoder_row_based.cc (right): https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/base/decod... remoting/base/decoder_row_based.cc:66: if (!screen_size_.isZero()) { On 2012/02/17 23:42:17, Wez wrote: > isZero() -> isEmpty() Done. https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/base/decod... remoting/base/decoder_row_based.cc:81: // The input buffer is a linear vector of pixel data. On 2012/02/17 23:42:17, Wez wrote: > nit: This comment doesn't really add anything to the following lines Done. https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/base/decod... remoting/base/decoder_row_based.cc:86: // Access the pixel buffer directly. On 2012/02/17 23:42:17, Wez wrote: > nit: This comment doesn't really add anything Done. https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/base/decod... remoting/base/decoder_row_based.cc:152: LOG(WARNING) << "Invalid clipping area received."; On 2012/02/17 23:42:17, Wez wrote: > If this is never valid then consider a DCHECK, or NOTREACHED This should never happen but this data arrives from the network so we should be able to handle any sort of bogus data. https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/base/decod... remoting/base/decoder_row_based.cc:205: // not match |screen_size_|. On 2012/02/17 23:42:17, Wez wrote: > nit: Consider changing this to a comment bemoaning the lack of scaling, rather > than a statement of intent to implement it. Or add a feature-tracking bug... ;) Done. https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/base/decod... File remoting/base/decoder_vp8.cc (right): https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/base/decod... remoting/base/decoder_vp8.cc:149: updated_region_.op(ScaleRect(clip_area, view_size, screen_size_), On 2012/02/17 23:42:17, Wez wrote: > This isn't strictly correct, I don't think. It'll cause pixels that *affect* > the clip-area, but which also affect pixels outside the clip-area in the scaled > view, not to be updated, leading to visual artefacts. Yes, but leaving such pixels invalidated will lead to painting in response to each and every packet. I'd say we should be able to tolerate such artifacts given that they can happen only if: - The view is scaled. - The clipping area does not cover the whole screen. Actually, in the current implementation it should not happen because if the view is scaled we see the whole screen and vise versa. Also the artifacts will be on the borders of the view area. https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/base/util.cc File remoting/base/util.cc (right): https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/base/util.... remoting/base/util.cc:222: // Use floating point to up/down scale more accurately. On 2012/02/17 23:42:17, Wez wrote: > Given that the results are still integer, how is this more accurate than the > existing code? Up-scaling didn't work properly in all cases with the previous version of the code. We need up-scaling for validating the clipping area when scaling the view. https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/client/fra... File remoting/client/frame_consumer.h (right): https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/client/fra... remoting/client/frame_consumer.h:26: // not to the clipping area. On 2012/02/17 23:42:17, Wez wrote: > Can you rephrase this comment to be more in keeping with other Chromium > comments, e.g: > > // Accepts a buffer and update region to be painted to the screen. > // ... > void PaintBuffer() Done. https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/client/fra... remoting/client/frame_consumer.h:29: pp::ImageData* buffer, On 2012/02/17 23:42:17, Wez wrote: > Hmmm. We really want a PP_Resource equivalent of scoped_ptr<pp::ImageData>. > Really, to keep this interface abstract, we should not use pp::IMageData here, > but keep that an implementation detail of PepperView. Well, we can introduce a wrapper object that carries a single buffer along with it's size, position within the frame, scaling factor and updated region. I.e. it will completely describe a single incremental update of the screen. This will also answer the previous question about |clip_area|. I'd like to defer any changed like that till a later CL though. :-) https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/client/fra... remoting/client/frame_consumer.h:35: virtual void ReturnBuffer(pp::ImageData* buffer) = 0; On 2012/02/17 23:42:17, Wez wrote: > scoped_ptr<>? No, this pointer does not carry ownership. The consumer keeps list of allocated buffers (and therefore ownership) and is responsible for freeing those buffers. The reason for this scheme is how PepperView shutdown works. https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/client/fra... remoting/client/frame_consumer.h:38: virtual void SetScreenSize(const SkISize& screen_size) = 0; On 2012/02/17 23:42:17, Wez wrote: > This is the only API with a Chromoting-specific name, but it's function isn't > actually Chromoting-specific - it just informs the FrameConsumer of the original > dimensions of the media. > > How about SetSourceSize(), SetOriginalSize() or SetNaturalSize()? SetSourceSize(). Done. https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/client/fra... File remoting/client/frame_consumer_proxy.cc (right): https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/client/fra... remoting/client/frame_consumer_proxy.cc:67: frame_consumer_ = NULL; On 2012/02/17 23:42:17, Wez wrote: > WHat happens to buffers in-flight when Detach() is called? The buffers will be deallocated synchronously within PepperView::TearDown. The way it currently works is: 1. ChromotingInstance's destructor is called. It does a bunch of things including synchronous waiting for the network connection shutdown. Among other things it calls PepperView::TearDown. 2. PepperView::TearDown notifies the producer that the queue of buffers has to be flushed. The producer queues a bunch of ReturnBuffer tasks. 3. Once tasks are queued, PepperView frees the buffers. 4. Detach() happens a bit later. 5. Some time later the tasks queued by the producer arrive at ConsumerProxy and ignored. This is not very clean shutdown sequence. It is modeled using the network connection shutdown as an example. https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/client/fra... File remoting/client/frame_producer.h (right): https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/client/fra... remoting/client/frame_producer.h:25: // the consumer. On 2012/02/17 23:42:17, Wez wrote: > nit: "Request that all pending buffers be returned to the consumer via > ReturnBuffer"? > > Consider renaming this to ReturnBuffers() or RequestReturnBuffers(). Done. https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/client/fra... remoting/client/frame_producer.h:33: // The passed buffer must be large enough to hold the whole clipping area. On 2012/02/17 23:42:17, Wez wrote: > I think this method needs a better name, and description! > > The method is used to add output buffers for the producer to use. Although > there is a queue in the implementation, it's not strictly necessary for the > producer to use a queue, I don't think. > > How about AddOutputBuffer()? AddBuffer() because no user method calls these buffers output buffers. https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/client/fra... remoting/client/frame_producer.h:39: // the frame. On 2012/02/17 23:42:17, Wez wrote: > nit: Comment style, e.g: > > "Requests that the specified region of the frame be refreshed as soon as > possible" Done. https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/client/fra... remoting/client/frame_producer.h:40: virtual void Invalidate(const SkRegion& region) = 0; On 2012/02/17 23:42:17, Wez wrote: > Consider calling this RequestRegion() or InvalidateRegion(). Done. https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/client/fra... remoting/client/frame_producer.h:44: // all enqueued buffers to the consumer for reuse or reallocation. On 2012/02/17 23:42:17, Wez wrote: > nit: Comment style, e.g: > > "Notifies the producer of changes to the output view size or visible (clip) > area." Done. https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/client/fra... remoting/client/frame_producer.h:45: virtual void SetView(const SkISize& view_size, const SkIRect& clip_area) = 0; On 2012/02/17 23:42:17, Wez wrote: > "View" is a Pepper term; in abstract terms this sets the output size, and > visible/clip area. How about SetOutputSizeAndClip()? It should be SetScalingFactorAndClipArea() then. I think it is unnecessary long and is as informative as SetView() is. "View" is a pretty generic term. It is used in Pepper API. It is used in a bunch of other cases. https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/client/plu... File remoting/client/plugin/pepper_view.cc (right): https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/client/plu... remoting/client/plugin/pepper_view.cc:84: // The producer should now return any pending buffers. On 2012/02/17 23:42:17, Wez wrote: > This comment doesn't seem to describe the line that follows it? Done. https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/client/plu... remoting/client/plugin/pepper_view.cc:93: done_event.Wait(); On 2012/02/17 23:42:17, Wez wrote: > Hmmm. It's a real shame to have to block here... Yes. The same approach is used when tearing down the network connection, so I figured when don't have a better choice. https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/client/plu... remoting/client/plugin/pepper_view.cc:106: if (producer_disabled_ && !clip_area_.isEmpty()) { On 2012/02/17 23:42:17, Wez wrote: > We should just strip out the static-fill code, I think... Probably, but not in this CL. :-) https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/client/plu... remoting/client/plugin/pepper_view.cc:189: bool updated = false; On 2012/02/17 23:42:17, Wez wrote: > Can you give this a more helpful name, e.g. size_or_clip_changed? "Size and Clip" constitute the view, so a called it "view_changed". I also removed the return value since it is not used any more. https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/client/plu... remoting/client/plugin/pepper_view.cc:191: // Prevent accidental upscaling. On 2012/02/17 23:42:17, Wez wrote: > nit: It's more that we don't currently support up-scaling. It is more than that. Currently it is the only way to ensure that we will not upscale by one or two pixels because JS code cannot estimate browser page zoom exactly. https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/client/plu... remoting/client/plugin/pepper_view.cc:231: // The data in the buffer is useless if the scale factor has changed. On 2012/02/17 23:42:17, Wez wrote: > Not strictly true; we could render what we have and thereby lag behind less > noticably in the case of a gradual resize operation? I added a TODO comment. https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/client/plu... remoting/client/plugin/pepper_view.cc:268: // Submit an update of desktop size to Javascript. On 2012/02/17 23:42:17, Wez wrote: > nit: Suggest "Notify JavaScript of the change in size" Done. https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/client/plu... remoting/client/plugin/pepper_view.cc:282: LOG(WARNING) << "Not enough memory for backing store."; On 2012/02/17 23:42:17, Wez wrote: > nit: "backing store" -> "frame buffers" Done. https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/client/plu... remoting/client/plugin/pepper_view.cc:325: // merge all pending changes into a single buffer and then apply it to On 2012/02/17 23:42:17, Wez wrote: > nit: Suggest "this code should apply all pending changes to the screen" Done. https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/client/plu... remoting/client/plugin/pepper_view.cc:336: base::Time start_time = base::Time::Now(); On 2012/02/17 23:42:17, Wez wrote: > Gahhh. We should update these stats gathering points to use TimeTicks(), > really. ... in a separate CL. https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/client/plu... remoting/client/plugin/pepper_view.cc:344: // to be painted. On 2012/02/17 23:42:17, Wez wrote: > How does this comment relate to the intersect() call below? Well, it explains why the intersect() call below is needed. The call literally does what the comment say: it figures out what is the portion of of the rect needs to be painted and skips the rect entirely if it doesn't fit the new clipping area. https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/client/plu... remoting/client/plugin/pepper_view.cc:349: rect.offset(- clip_area.left(), - clip_area.top()); On 2012/02/17 23:42:17, Wez wrote: > nit: Not 100% sure, but I think the '-' should not be separated from the terms, > since they're the unary form. Done. https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/client/plu... remoting/client/plugin/pepper_view.cc:352: // point here is the offset from the source rect. Why? On 2012/02/17 23:42:17, Wez wrote: > It is within our power to fix the Graphics2D comments. ;) As you can imagine I didn't write this comment. WE could fix the comment and Graphics2D but let's keep more changes to this code out of this CL. https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/client/plu... remoting/client/plugin/pepper_view.cc:359: // Flush the updated areas ro the screen. On 2012/02/17 23:42:17, Wez wrote: > typo: ro -> to Done. https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/client/plu... remoting/client/plugin/pepper_view.cc:371: // result value. On 2012/02/17 23:42:17, Wez wrote: > This is ugly. We should be able to clean this up reasonably easily, I think? I inherited this code. I'd like to defer any such clean up until a better time. https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/client/plu... remoting/client/plugin/pepper_view.cc:380: ignore_result(task.release()); On 2012/02/17 23:42:17, Wez wrote: > This doesn't look right. Shouldn't we be release()ing task for > CompletionCallbackClosureAdapter to destroy? As it stands this code looks like > it'll leak the task. Looks OK to me. If completion is pending, CompletionCallbackClosureAdapter will be called and the closure will be deleted there. Otherwise scoped_ptr will delete it. https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/client/plu... File remoting/client/plugin/pepper_view.h (right): https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/client/plu... remoting/client/plugin/pepper_view.h:117: // True if there is pending paint commands in Pepper's queue. This is set to On 2012/02/17 23:42:17, Wez wrote: > nit: is -> are > > Consider rewording: True if there is already a Flush() pending on the Graphics2D > context. Done. https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/client/plu... remoting/client/plugin/pepper_view.h:121: // When true no new work should be posted to the producer. On 2012/02/17 23:42:17, Wez wrote: > Doesn't work come from the RectangleUpdateDecoder? Done: work -> buffers. https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/client/rec... File remoting/client/rectangle_update_decoder.cc (right): https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/client/rec... remoting/client/rectangle_update_decoder.cc:171: // updated because they haven't been validated by RenderFrame() yet. On 2012/02/17 23:42:17, Wez wrote: > This sentence sounds the wrong way around? Yeah. I was trying to say that if the scaling is the same we don't need to do anything special. But it is better to say that by not saying anything. :-)
My last set of comments, I expect. ;) https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/base/decod... File remoting/base/decoder_vp8.cc (right): https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/base/decod... remoting/base/decoder_vp8.cc:149: updated_region_.op(ScaleRect(clip_area, view_size, screen_size_), On 2012/02/21 23:00:44, alexeypa wrote: > On 2012/02/17 23:42:17, Wez wrote: > > This isn't strictly correct, I don't think. It'll cause pixels that *affect* > > the clip-area, but which also affect pixels outside the clip-area in the > scaled > > view, not to be updated, leading to visual artefacts. > > Yes, but leaving such pixels invalidated will lead to painting in response to > each and every packet. I'd say we should be able to tolerate such artifacts > given that they can happen only if: > > - The view is scaled. > - The clipping area does not cover the whole screen. > > Actually, in the current implementation it should not happen because if the > view is scaled we see the whole screen and vise versa. Unless the view is scaled because of page zoom. ;) > Also the artifacts will be on the borders of the view area. I can't see any way to get this right besides storing separate invalid regions in client and host coordinates. :-/ https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/base/util.cc File remoting/base/util.cc (right): https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/base/util.... remoting/base/util.cc:222: // Use floating point to up/down scale more accurately. On 2012/02/21 23:00:44, alexeypa wrote: > On 2012/02/17 23:42:17, Wez wrote: > > Given that the results are still integer, how is this more accurate than the > > existing code? > > Up-scaling didn't work properly in all cases with the previous version of the > code. We need up-scaling for validating the clipping area when scaling the view. Do remember in what way it was broken? I'd prefer that we fix the integer arithmetic rather than start reintroducing floating-point code. https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/client/fra... File remoting/client/frame_consumer.h (right): https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/client/fra... remoting/client/frame_consumer.h:29: pp::ImageData* buffer, On 2012/02/21 23:00:44, alexeypa wrote: > On 2012/02/17 23:42:17, Wez wrote: > > Hmmm. We really want a PP_Resource equivalent of scoped_ptr<pp::ImageData>. > > Really, to keep this interface abstract, we should not use pp::IMageData here, > > but keep that an implementation detail of PepperView. > > Well, we can introduce a wrapper object that carries a single buffer along with > it's size, position within the frame, scaling factor and updated region. I.e. it > will completely describe a single incremental update of the screen. > > This will also answer the previous question about |clip_area|. I'd like to defer > any changed like that till a later CL though. :-) OK! https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/client/fra... File remoting/client/frame_consumer_proxy.cc (right): https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/client/fra... remoting/client/frame_consumer_proxy.cc:67: frame_consumer_ = NULL; On 2012/02/21 23:00:44, alexeypa wrote: > On 2012/02/17 23:42:17, Wez wrote: > > WHat happens to buffers in-flight when Detach() is called? > > The buffers will be deallocated synchronously within PepperView::TearDown. The > way it currently works is: > > 1. ChromotingInstance's destructor is called. It does a bunch of things > including synchronous waiting for the network connection shutdown. Among other > things it calls PepperView::TearDown. > 2. PepperView::TearDown notifies the producer that the queue of buffers has to > be flushed. The producer queues a bunch of ReturnBuffer tasks. > 3. Once tasks are queued, PepperView frees the buffers. > 4. Detach() happens a bit later. > 5. Some time later the tasks queued by the producer arrive at ConsumerProxy and > ignored. > > This is not very clean shutdown sequence. It is modeled using the network > connection shutdown as an example. Sounds like this needs at least a brief comment to explain that by the time we Detach() all buffers will have been returned to the FrameConsumer. https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/client/fra... File remoting/client/frame_producer.h (right): https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/client/fra... remoting/client/frame_producer.h:33: // The passed buffer must be large enough to hold the whole clipping area. On 2012/02/21 23:00:44, alexeypa wrote: > On 2012/02/17 23:42:17, Wez wrote: > > I think this method needs a better name, and description! > > > > The method is used to add output buffers for the producer to use. Although > > there is a queue in the implementation, it's not strictly necessary for the > > producer to use a queue, I don't think. > > > > How about AddOutputBuffer()? > > AddBuffer() because no user method calls these buffers output buffers. Wouldn't DrawBuffer be more appropriate, then, since the buffer gets drawn? https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/client/fra... remoting/client/frame_producer.h:45: virtual void SetView(const SkISize& view_size, const SkIRect& clip_area) = 0; On 2012/02/21 23:00:44, alexeypa wrote: > On 2012/02/17 23:42:17, Wez wrote: > > "View" is a Pepper term; in abstract terms this sets the output size, and > > visible/clip area. How about SetOutputSizeAndClip()? > > It should be SetScalingFactorAndClipArea() then. I think it is unnecessary long > and is as informative as SetView() is. "View" is a pretty generic term. It is > used in Pepper API. It is used in a bunch of other cases. The API doesn't take a scaling factor, it takes an output size and an output clip area; hence my suggesting SetOutputSizeAndClip(). The problem with View is that it's Pepper-specific, and doesn't mean output size and clip even in that context. https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/client/plu... File remoting/client/plugin/pepper_view.cc (right): https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/client/plu... remoting/client/plugin/pepper_view.cc:106: if (producer_disabled_ && !clip_area_.isEmpty()) { On 2012/02/21 23:00:44, alexeypa wrote: > On 2012/02/17 23:42:17, Wez wrote: > > We should just strip out the static-fill code, I think... > > Probably, but not in this CL. :-) Unless it simplifies the CL...? https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/client/plu... remoting/client/plugin/pepper_view.cc:191: // Prevent accidental upscaling. On 2012/02/21 23:00:44, alexeypa wrote: > On 2012/02/17 23:42:17, Wez wrote: > > nit: It's more that we don't currently support up-scaling. > > It is more than that. Currently it is the only way to ensure that we will not > upscale by one or two pixels because JS code cannot estimate browser page zoom > exactly. Which wouldn't be a problem if we supported up-scaling. ;) (And with page zoom enabled we can end up up-scaling by much more than a couple of pixels.) https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/client/plu... remoting/client/plugin/pepper_view.cc:344: // to be painted. On 2012/02/21 23:00:44, alexeypa wrote: > On 2012/02/17 23:42:17, Wez wrote: > > How does this comment relate to the intersect() call below? > > Well, it explains why the intersect() call below is needed. The call literally > does what the comment say: it figures out what is the portion of of the rect > needs to be painted and skips the rect entirely if it doesn't fit the new > clipping area. The comment refers to |clip_area_| and |clip_area|, but not to |region|, which is the thing that is (effectively) being intersected with |clip_area_|. It's not clear from the comment what the intersect is intended to achieve. I think you're just saying that we need to re-clip |region| in case the clip area has changed since the buffer was queued for painting? https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/client/plu... remoting/client/plugin/pepper_view.cc:352: // point here is the offset from the source rect. Why? On 2012/02/21 23:00:44, alexeypa wrote: > On 2012/02/17 23:42:17, Wez wrote: > > It is within our power to fix the Graphics2D comments. ;) > > As you can imagine I didn't write this comment. WE could fix the comment and > Graphics2D but let's keep more changes to this code out of this CL. Indeed. Not sure where my comment on this sprang from... https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/client/plu... remoting/client/plugin/pepper_view.cc:371: // result value. On 2012/02/21 23:00:44, alexeypa wrote: > On 2012/02/17 23:42:17, Wez wrote: > > This is ugly. We should be able to clean this up reasonably easily, I think? > > I inherited this code. I'd like to defer any such clean up until a better time. SGTM https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/client/plu... remoting/client/plugin/pepper_view.cc:380: ignore_result(task.release()); On 2012/02/21 23:00:44, alexeypa wrote: > On 2012/02/17 23:42:17, Wez wrote: > > This doesn't look right. Shouldn't we be release()ing task for > > CompletionCallbackClosureAdapter to destroy? As it stands this code looks > like > > it'll leak the task. > > Looks OK to me. If completion is pending, CompletionCallbackClosureAdapter will > be called and the closure will be deleted there. Otherwise scoped_ptr will > delete it. Yes, you're right! https://chromiumcodereview.appspot.com/9331003/diff/32001/remoting/client/fra... File remoting/client/frame_consumer.h (right): https://chromiumcodereview.appspot.com/9331003/diff/32001/remoting/client/fra... remoting/client/frame_consumer.h:25: // pixels within |region| are painted. The function assumes that the passed nit: within |region| and |clip_area|? Or must |region| be pre-clipped? https://chromiumcodereview.appspot.com/9331003/diff/32001/remoting/client/plu... File remoting/client/plugin/pepper_view.cc (right): https://chromiumcodereview.appspot.com/9331003/diff/32001/remoting/client/plu... remoting/client/plugin/pepper_view.cc:229: // TODO(alexeypa): We could rescale and draw it to reduce the perception lag nit: Don't even need to re-scale; we could just draw what we currently have at the wrong resolution so that things lag a little rather than stalling. https://chromiumcodereview.appspot.com/9331003/diff/32001/remoting/client/plu... remoting/client/plugin/pepper_view.cc:229: // TODO(alexeypa): We could rescale and draw it to reduce the perception lag typo: perception -> perceived typo: wating -> waiting typo: preperly -> properly or correctly
PTAL. I've removed Jamie from the list of reviewers because I reverted all JS changes. https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/base/decod... File remoting/base/decoder_vp8.cc (right): https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/base/decod... remoting/base/decoder_vp8.cc:149: updated_region_.op(ScaleRect(clip_area, view_size, screen_size_), On 2012/02/23 00:11:09, Wez wrote: > Unless the view is scaled because of page zoom. ;) Currently we scale it back to fit the screen even if it is zoomed. > > Also the artifacts will be on the borders of the view area. > > I can't see any way to get this right besides storing separate invalid regions > in client and host coordinates. :-/ One way of doing it is to store the invalid region in output coordinates, separately from the decoder. The decoder will post updates to the region at it receives frames. The client will schedule drawing. This will probably work but it will create additional round trip between the decoder and the client. Right now I'm included to say - let someone notice the artifacts, file a bug and then we'll fix it. https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/base/util.cc File remoting/base/util.cc (right): https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/base/util.... remoting/base/util.cc:222: // Use floating point to up/down scale more accurately. On 2012/02/23 00:11:09, Wez wrote: > Do remember in what way it was broken? I'd prefer that we fix the integer > arithmetic rather than start reintroducing floating-point code. Sorry, I don't remember. I do remember that I hit in some extreme case and the error made sense at the time. :-) I think floating point is not too bad, given that it is used in the decoder and, in the future, by the RGB-to-YUV converter/scaler anyway. https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/client/fra... File remoting/client/frame_consumer_proxy.cc (right): https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/client/fra... remoting/client/frame_consumer_proxy.cc:67: frame_consumer_ = NULL; On 2012/02/23 00:11:09, Wez wrote: > Sounds like this needs at least a brief comment to explain that by the time we > Detach() all buffers will have been returned to the FrameConsumer. It is explained in PepperView::TearDown. https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/client/fra... File remoting/client/frame_producer.h (right): https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/client/fra... remoting/client/frame_producer.h:33: // The passed buffer must be large enough to hold the whole clipping area. On 2012/02/23 00:11:09, Wez wrote: > On 2012/02/21 23:00:44, alexeypa wrote: > > AddBuffer() because no user method calls these buffers output buffers. > Wouldn't DrawBuffer be more appropriate, then, since the buffer gets drawn? FrameProducer::AddBuffer -> FrameProducer::DrawBuffer FrameConsumer::PaintBuffer -> FrameConsumer::ApplyBuffer https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/client/fra... remoting/client/frame_producer.h:45: virtual void SetView(const SkISize& view_size, const SkIRect& clip_area) = 0; On 2012/02/23 00:11:09, Wez wrote: > On 2012/02/21 23:00:44, alexeypa wrote: > > On 2012/02/17 23:42:17, Wez wrote: > > > "View" is a Pepper term; in abstract terms this sets the output size, and > > > visible/clip area. How about SetOutputSizeAndClip()? > > > > It should be SetScalingFactorAndClipArea() then. I think it is unnecessary > long > > and is as informative as SetView() is. "View" is a pretty generic term. It is > > used in Pepper API. It is used in a bunch of other cases. > > The API doesn't take a scaling factor, it takes an output size and an output > clip area; hence my suggesting SetOutputSizeAndClip(). > > The problem with View is that it's Pepper-specific, and doesn't mean output size > and clip even in that context. Done. https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/client/plu... File remoting/client/plugin/pepper_view.cc (right): https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/client/plu... remoting/client/plugin/pepper_view.cc:106: if (producer_disabled_ && !clip_area_.isEmpty()) { On 2012/02/23 00:11:09, Wez wrote: > Unless it simplifies the CL...? OK. https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/client/plu... remoting/client/plugin/pepper_view.cc:191: // Prevent accidental upscaling. On 2012/02/23 00:11:09, Wez wrote: > Which wouldn't be a problem if we supported up-scaling. ;) > > (And with page zoom enabled we can end up up-scaling by much more than a couple > of pixels.) Done as per our discussion. https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/client/plu... remoting/client/plugin/pepper_view.cc:344: // to be painted. On 2012/02/23 00:11:09, Wez wrote: > The comment refers to |clip_area_| and |clip_area|, but not to |region|, which > is the thing that is (effectively) being intersected with |clip_area_|. It's > not clear from the comment what the intersect is intended to achieve. I think > you're just saying that we need to re-clip |region| in case the clip area has > changed since the buffer was queued for painting? I've reworded the comment. https://chromiumcodereview.appspot.com/9331003/diff/32001/remoting/client/fra... File remoting/client/frame_consumer.h (right): https://chromiumcodereview.appspot.com/9331003/diff/32001/remoting/client/fra... remoting/client/frame_consumer.h:25: // pixels within |region| are painted. The function assumes that the passed On 2012/02/23 00:11:09, Wez wrote: > nit: within |region| and |clip_area|? Or must |region| be pre-clipped? Reworded the comment to mention that only pixels within |region| and the current clipping area are painted. https://chromiumcodereview.appspot.com/9331003/diff/32001/remoting/client/plu... File remoting/client/plugin/pepper_view.cc (right): https://chromiumcodereview.appspot.com/9331003/diff/32001/remoting/client/plu... remoting/client/plugin/pepper_view.cc:229: // TODO(alexeypa): We could rescale and draw it to reduce the perception lag On 2012/02/23 00:11:09, Wez wrote: > typo: perception -> perceived > typo: wating -> waiting > typo: preperly -> properly or correctly Done. https://chromiumcodereview.appspot.com/9331003/diff/32001/remoting/client/plu... remoting/client/plugin/pepper_view.cc:229: // TODO(alexeypa): We could rescale and draw it to reduce the perception lag On 2012/02/23 00:11:09, Wez wrote: > nit: Don't even need to re-scale; we could just draw what we currently have at > the wrong resolution so that things lag a little rather than stalling. Done.
https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/base/util.cc File remoting/base/util.cc (right): https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/base/util.... remoting/base/util.cc:222: // Use floating point to up/down scale more accurately. On 2012/02/23 17:10:33, alexeypa wrote: > On 2012/02/23 00:11:09, Wez wrote: > > Do remember in what way it was broken? I'd prefer that we fix the integer > > arithmetic rather than start reintroducing floating-point code. > > Sorry, I don't remember. I do remember that I hit in some extreme case and the > error made sense at the time. :-) OK; there's no intrinsic reason why doing the integer calculation should be any less accurate than the float one. > I think floating point is not too bad, given that it is used in the decoder and, > in the future, by the RGB-to-YUV converter/scaler anyway. Neither the decoder not converter/scaler should be using floating point either, as far as I'm aware... https://chromiumcodereview.appspot.com/9331003/diff/39001/remoting/client/fra... File remoting/client/frame_consumer.h (right): https://chromiumcodereview.appspot.com/9331003/diff/39001/remoting/client/fra... remoting/client/frame_consumer.h:27: // having |view_size| dimentions. typo: dimensions https://chromiumcodereview.appspot.com/9331003/diff/39001/remoting/client/plu... File remoting/client/plugin/pepper_view.h (right): https://chromiumcodereview.appspot.com/9331003/diff/39001/remoting/client/plu... remoting/client/plugin/pepper_view.h:115: bool producer_disabled_; This flag is only set during teardown, so perhaps it would be better to call it |in_teardown| or similar? https://chromiumcodereview.appspot.com/9331003/diff/39001/remoting/webapp/cli... File remoting/webapp/client_session.js (left): https://chromiumcodereview.appspot.com/9331003/diff/39001/remoting/webapp/cli... remoting/webapp/client_session.js:446: nit: Looks like this file didn't get reverted cleanly. ;)
PTAL. Note that the diff was rebased so that could run it though the bots. https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/base/util.cc File remoting/base/util.cc (right): https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/base/util.... remoting/base/util.cc:222: // Use floating point to up/down scale more accurately. On 2012/02/23 22:59:26, Wez wrote: > OK; there's no intrinsic reason why doing the integer calculation should be any > less accurate than the float one. OK, here is the bug I was talking about. Say in_size.width() == 10, out_size.width() == 1000 and rect.right() == 10. The original formula gives you 1099. The floating point one - 1000. The bug is caused by the fact that we are adding (out_size.width() - 1), not (in_size.width() - 1), which would be correct. Now there are two ways to fix it: convert to floating point or fix the integer arithmetic. The former one is straightforward and easy to read. The latter one is more tricky to understand (which is proven by the bug) and less readable. I chose the former one for its readability and because this is not a performance critical path in the code. > Neither the decoder not converter/scaler should be using floating point either, > as far as I'm aware... We want use SSE which is the same thing as floating point from CPU point of view. CPU will have to swap FPU state anyway if either was used. https://chromiumcodereview.appspot.com/9331003/diff/39001/remoting/client/fra... File remoting/client/frame_consumer.h (right): https://chromiumcodereview.appspot.com/9331003/diff/39001/remoting/client/fra... remoting/client/frame_consumer.h:27: // having |view_size| dimentions. On 2012/02/23 22:59:26, Wez wrote: > typo: dimensions Done. https://chromiumcodereview.appspot.com/9331003/diff/39001/remoting/client/plu... File remoting/client/plugin/pepper_view.h (right): https://chromiumcodereview.appspot.com/9331003/diff/39001/remoting/client/plu... remoting/client/plugin/pepper_view.h:115: bool producer_disabled_; On 2012/02/23 22:59:26, Wez wrote: > This flag is only set during teardown, so perhaps it would be better to call it > |in_teardown| or similar? Done. https://chromiumcodereview.appspot.com/9331003/diff/39001/remoting/webapp/cli... File remoting/webapp/client_session.js (left): https://chromiumcodereview.appspot.com/9331003/diff/39001/remoting/webapp/cli... remoting/webapp/client_session.js:446: On 2012/02/23 22:59:26, Wez wrote: > nit: Looks like this file didn't get reverted cleanly. ;) Done.
LGTM, with the exception of ScaleRect(), which I'd really prefer remaining integer-only; see below. https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/base/util.cc File remoting/base/util.cc (right): https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/base/util.... remoting/base/util.cc:222: // Use floating point to up/down scale more accurately. On 2012/02/23 23:59:14, alexeypa wrote: > On 2012/02/23 22:59:26, Wez wrote: > > OK; there's no intrinsic reason why doing the integer calculation should be > any > > less accurate than the float one. > > OK, here is the bug I was talking about. Say in_size.width() == 10, > out_size.width() == 1000 and rect.right() == 10. The original formula gives you > 1099. The floating point one - 1000. > > The bug is caused by the fact that we are adding (out_size.width() - 1), not > (in_size.width() - 1), which would be correct. Correct. :) > Now there are two ways to fix it: > convert to floating point or fix the integer arithmetic. The former one is > straightforward and easy to read. The latter one is more tricky to understand > (which is proven by the bug) and less readable. I chose the former one for its > readability and because this is not a performance critical path in the code. We may have a large number of rectangles in an update, so on more limited platforms (e.g. ARM) the floating-point hit can be a problem. Since we're going from integer inputs to integer outputs, fixing my broken arithmetic seems preferably to jumping into and out of floating-point. > > Neither the decoder not converter/scaler should be using floating point > either, > > as far as I'm aware... > > We want use SSE which is the same thing as floating point from CPU point of > view. CPU will have to swap FPU state anyway if either was used. Our SSE code is fixed-point; or are you just referring to the actual registers that the instructions use?
FYI https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/base/util.cc File remoting/base/util.cc (right): https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/base/util.... remoting/base/util.cc:222: // Use floating point to up/down scale more accurately. On 2012/02/24 00:12:30, Wez wrote: > We may have a large number of rectangles in an update, so on more limited > platforms (e.g. ARM) the floating-point hit can be a problem. Since we're going > from integer inputs to integer outputs, fixing my broken arithmetic seems > preferably to jumping into and out of floating-point. I don't think anything can be called a performance critical path without first measuring it. :-) I did a couple micro-benchmarks last night. Floating point on x86 was noticeably worse than integer that surprised me - floating point version has twice less divisions. It appears that the compiler generates too many helper calls. So we were right on this one. x64 performance was the same which is not surprising at all. x64 is irrelevant to Chrome at the moment. I suspect floating point on ARM will actually be better because ARM uses software division for both integer and floating point (except that NEON has hardware approximation for getting the reciprocal to emulate floating point division). So two vs four divisions will probably show up. It was too late for installing Adroid NDK yesterday. :-) Given all that I'm reverting to the integer version, which seems to be a better choice so far. > Our SSE code is fixed-point; or are you just referring to the actual registers > that the instructions use? I meant the actual register state. Once SSE or FP is used CPU has load/store both FP and SSE state. (Yes, I'm aware of lazy FPU state loading :-) ).
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/alexeypa@chromium.org/9331003/47001
https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/base/util.cc File remoting/base/util.cc (right): https://chromiumcodereview.appspot.com/9331003/diff/18002/remoting/base/util.... remoting/base/util.cc:222: // Use floating point to up/down scale more accurately. On 2012/02/24 16:46:56, alexeypa wrote: > On 2012/02/24 00:12:30, Wez wrote: > > We may have a large number of rectangles in an update, so on more limited > > platforms (e.g. ARM) the floating-point hit can be a problem. Since we're > going > > from integer inputs to integer outputs, fixing my broken arithmetic seems > > preferably to jumping into and out of floating-point. > > I don't think anything can be called a performance critical path without first > measuring it. :-) > > I did a couple micro-benchmarks last night. Floating point on x86 was noticeably > worse than integer that surprised me - floating point version has twice less > divisions. It appears that the compiler generates too many helper calls. So we > were right on this one. Sorry, I should have pointed out that my comments on performance impact are based on past experience implementing this code for ARM & x86. > x64 performance was the same which is not surprising at > all. x64 is irrelevant to Chrome at the moment. Except on Linux. ;) > I suspect floating point on ARM will actually be better because ARM uses > software division for both integer and floating point (except that NEON has > hardware approximation for getting the reciprocal to emulate floating point > division). So two vs four divisions will probably show up. It was too late for > installing Adroid NDK yesterday. :-) If I remember correctly, integer division on ARM is considerably cheaper than floating-point but still considerably more expensive than other integer operations. You can work around that by approximating (a / b) as ((a * (2^k / b)) >> k), and choosing k to minimize rounding errors while fitting in your registers. > Given all that I'm reverting to the integer version, which seems to be a better > choice so far. OK. Thanks for the thorough investigation! > > Our SSE code is fixed-point; or are you just referring to the actual registers > > that the instructions use? > > I meant the actual register state. Once SSE or FP is used CPU has load/store > both FP and SSE state. (Yes, I'm aware of lazy FPU state loading :-) ). That's a valid concern, although I'd expect floating point execution time to outweigh load/store overhead on any hot code-paths. Again, something we can look into if we see performance issues. :)
Try job failure for 9331003-47001 (retry) on win_rel for step "unit_tests". It's a second try, previously, steps "unit_tests, browser_tests, ui_tests" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/alexeypa@chromium.org/9331003/47001
Change committed as 123573 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
