Bug 100042 – [chromium] Add textureWidth() and textureHeight() to WebVideoFrame

• Component: New Bugs
• Status: RESOLVED FIXED

[John Sheu]

[chromium] Add textureWidth() and textureHeight() to WebVideoFrame

[John Sheu]

Created attachment 170005 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[John Sheu]

Just an interface change at the moment.  I'd like to remove stride() eventually, but only after I land the Chromium-side changes as wekk,

[James Robinson]

Comment on attachment 170005 [details]
Patch

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

> Source/Platform/ChangeLog:8
> +        Additional information of the change such as approach, rationale. Please add per-function descriptions below (OOPS!).

You have to fill something out here.  For this, when would textureWidth/textureHight be different from the frame's width/height?  Why do we need separate fields?

[John Sheu]

Created attachment 170009 [details]
Patch

[John Sheu]

Please take a look here.

[James Robinson]

I can't find any equivalent in media/base/video_frame.h.  Perhaps there's some context here I'm missing?  Is there a crbug or chromium-side code review that goes along with this?

[John Sheu]

The Chromium bug lives at http://crbug.com/155416, and the Chromium-side change is WIP on my checkout.  I can't actually checkin for Chromium until this interface change lands, so this one's going first.  (As the interface change just adds two accessors, it's safe to do first.)

[Ami Fischman]

(In reply to comment #8)
> The Chromium bug lives at http://crbug.com/155416, and the Chromium-side change is WIP on my checkout.  I can't actually checkin for Chromium until this interface change lands, so this one's going first.  (As the interface change just adds two accessors, it's safe to do first.)

@sheu: actually, this is http://crbug.com/140509 :)  You taking that over? (that'd be great, since I haven't had time to touch it)

(155416 describes a symptom; 140509 describes the root cause and a plan for fixing it)

[James Robinson]

Comment on attachment 170009 [details]
Patch

Thanks for the info. Slavomir (cc'd) is working on moving the dependency from media -> cc down to not need the WebVideoFrame intermediate data structure, so hopefully this won't be needed in WebKit for long.  But for now it definitely is.

[John Sheu]

Created attachment 170491 [details]
Patch

[John Sheu]

Updated patch.

I have a question on naming in the interface.  To wit: is visibleX/visibleY/visibleWidth/visibleHeight the right name to use for the added interface functions, in WebKit style?

[James Robinson]

(In reply to comment #12)
> Updated patch.
> 
> I have a question on naming in the interface.  To wit: is visibleX/visibleY/visibleWidth/visibleHeight the right name to use for the added interface functions, in WebKit style?

The naming is correct - accessors should match the variable name they are accessing - but it'd be better to have the whole thing be one accessor that returns type WebRect instead of 4 individual methods.

[John Sheu]

Created attachment 170523 [details]
Patch

[Peter Beverloo (cr-android ews)]

Comment on attachment 170523 [details]
Patch

Attachment 170523 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/14554451

[John Sheu]

Created attachment 170530 [details]
Patch

[James Robinson]

Comment on attachment 170530 [details]
Patch

Great, R=me

[WebKit Review Bot]

Comment on attachment 170530 [details]
Patch

Clearing flags on attachment: 170530

Committed r132448: <http://trac.webkit.org/changeset/132448>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.