Use the supplied memory for YUV decoding.

Use the supplied memory for YUV decoding.

As some kind of optimization the yuv decoder replaced some pointers
to point to decoder-internal memory when an API user asked for
yuv data. That often worked, but failed if the data needed to
outlive the decoder. It was also a violation of the API contract.

This replaces that with a buffer copy. It will be slower but will avoid
random bitmap errors that also happened randomly on cq try bots.

This is a stopgap solution while a better solution is designed, coded and
tested.

BUG=455235
R=sugoi@chromium.org,dongseong.hwang@intel.com

Committed: https://crrev.com/a86ffa06e4415b155a96b9513ed2f18e0d9deaf6
Cr-Commit-Position: refs/heads/master@{#314865}

[dshwang, 2015-02-04 15:58:03]

dongseong.hwang@intel.com changed reviewers:
+ dongseong.hwang@intel.com

[dshwang, 2015-02-04 15:58:04]

https://codereview.chromium.org/901573004/diff/1/media/filters/skcanvas_video...
File media/filters/skcanvas_video_renderer.cc (right):

https://codereview.chromium.org/901573004/diff/1/media/filters/skcanvas_video...
media/filters/skcanvas_video_renderer.cc:401: memcpy(out_line, in_line,
bytes_to_copy_per_line);
I think skia doesn't alloc planes[plane] so memcpy might screw up stack.

[sugoi1, 2015-02-04 16:01:24]

sugoi@chromium.org changed reviewers:
+ sugoi@chromium.org

[sugoi1, 2015-02-04 16:01:24]

https://codereview.chromium.org/901573004/diff/1/media/filters/skcanvas_video...
File media/filters/skcanvas_video_renderer.cc (right):

https://codereview.chromium.org/901573004/diff/1/media/filters/skcanvas_video...
media/filters/skcanvas_video_renderer.cc:401: memcpy(out_line, in_line,
bytes_to_copy_per_line);
On 2015/02/04 15:58:04, dshwang wrote:
> I think skia doesn't alloc planes[plane] so memcpy might screw up stack.

The memory should have been allocated here:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/skia/s...

[sugoi1, 2015-02-04 18:08:48]

https://codereview.chromium.org/901573004/diff/1/media/filters/skcanvas_video...
File media/filters/skcanvas_video_renderer.cc (right):

https://codereview.chromium.org/901573004/diff/1/media/filters/skcanvas_video...
media/filters/skcanvas_video_renderer.cc:379: sizes[plane].set(size.width(),
size.height());
FYI, here, if you want to have the same stride in skia memory and in your Video
Frame you could do:
sizes[plane].set((row_bytes && planes) ? size.width() : frame_->stride(plane),
size.height());

https://codereview.chromium.org/901573004/diff/1/media/filters/skcanvas_video...
media/filters/skcanvas_video_renderer.cc:398: out_line_stride < in_line_stride ?
out_line_stride : in_line_stride;
I don't want to add too much complexity to this code, but you could do this in a
single memcpy call if out_line_stride == in_line_stride.

[Daniel Bratell, 2015-02-05 13:08:37]

Seems from the discussions that we should use this as a stopgap solution while
dshwang and sugoi et al work on a more efficient API so please review this for
landing in mainline.

https://codereview.chromium.org/901573004/diff/1/media/filters/skcanvas_video...
File media/filters/skcanvas_video_renderer.cc (right):

https://codereview.chromium.org/901573004/diff/1/media/filters/skcanvas_video...
media/filters/skcanvas_video_renderer.cc:379: sizes[plane].set(size.width(),
size.height());
On 2015/02/04 18:08:47, sugoi1 wrote:
> FYI, here, if you want to have the same stride in skia memory and in your
Video
> Frame you could do:
> sizes[plane].set((row_bytes && planes) ? size.width() : frame_->stride(plane),
> size.height());

It would break the contract though, and getting different sizes depending on if
you send in null for planes/rowstrides or not could probably cause future
confusion and bugs.

Changing the API is a bit tricky but it would have been good if it had output
strides as well as sizes.

https://codereview.chromium.org/901573004/diff/1/media/filters/skcanvas_video...
media/filters/skcanvas_video_renderer.cc:398: out_line_stride < in_line_stride ?
out_line_stride : in_line_stride;
On 2015/02/04 18:08:47, sugoi1 wrote:
> I don't want to add too much complexity to this code, but you could do this in
a
> single memcpy call if out_line_stride == in_line_stride.

Done.

https://codereview.chromium.org/901573004/diff/1/media/filters/skcanvas_video...
media/filters/skcanvas_video_renderer.cc:401: memcpy(out_line, in_line,
bytes_to_copy_per_line);
On 2015/02/04 16:01:24, sugoi1 wrote:
> On 2015/02/04 15:58:04, dshwang wrote:
> > I think skia doesn't alloc planes[plane] so memcpy might screw up stack.
> 
> The memory should have been allocated here:
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/skia/s...

Acknowledged.

[dshwang, 2015-02-05 14:33:40]

dongseong.hwang@intel.com changed reviewers:
+ dalecurtis@chromium.org

[dshwang, 2015-02-05 14:33:41]

nonowner lgtm

+dalecurtis as media owner

[sugoi1, 2015-02-05 15:18:18]

Also non owner lgtm

https://codereview.chromium.org/901573004/diff/1/media/filters/skcanvas_video...
File media/filters/skcanvas_video_renderer.cc (right):

https://codereview.chromium.org/901573004/diff/1/media/filters/skcanvas_video...
media/filters/skcanvas_video_renderer.cc:379: sizes[plane].set(size.width(),
size.height());
On 2015/02/05 13:08:37, Daniel Bratell wrote:
> On 2015/02/04 18:08:47, sugoi1 wrote:
> > FYI, here, if you want to have the same stride in skia memory and in your
> Video
> > Frame you could do:
> > sizes[plane].set((row_bytes && planes) ? size.width() :
frame_->stride(plane),
> > size.height());
> 
> It would break the contract though, and getting different sizes depending on
if
> you send in null for planes/rowstrides or not could probably cause future
> confusion and bugs.
> 
> Changing the API is a bit tricky but it would have been good if it had output
> strides as well as sizes.

We're working on a new API. This should allow us to fix this case properly when
we all agree on it:
https://codereview.chromium.org/863053002/

[DaleCurtis, 2015-02-05 19:12:44]

lgtm

[sugoi1, 2015-02-05 19:13:33]

The CQ bit was checked by sugoi@chromium.org

[commit-bot: I haz the power, 2015-02-05 19:14:46]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/901573004/20001

[commit-bot: I haz the power, 2015-02-05 20:16:01]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-02-05 20:17:16]

Patchset 2 (id:??) landed as
https://crrev.com/a86ffa06e4415b155a96b9513ed2f18e0d9deaf6
Cr-Commit-Position: refs/heads/master@{#314865}