Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1117423002:
media: Let VideoFrame carry more than one native texture. (Closed)
|
Created:
5 years, 7 months ago by Daniele Castagna Modified:
5 years, 7 months ago CC:
avayvod+watch_chromium.org, cc-bugs_chromium.org, chromium-reviews, darin-cc_chromium.org, emircan, feature-media-reviews_chromium.org, hclam+watch_chromium.org, hguihot+watch_chromium.org, hubbe+watch_chromium.org, imcheng+watch_chromium.org, jam, jasonroberts+watch_google.com, mcasas+watch_chromium.org, mcasas, miu+watch_chromium.org, mkwst+moarreviews-renderer_chromium.org, mlamouri+watch-content_chromium.org, posciak+watch_chromium.org, reveman, wjia+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionmedia: Let VideoFrame carry more than one native texture.
The goal of this patch is to be able to use VideoFrame to be
created and to transport three R8 textures that represent a YUV
frame.
A new enum VideoFrame::TextureFormat has been added in order to
determine how the textures attached to the VideoFrame should be
interpreted when the VideoFrame::Format is NATIVE_TEXTURE.
BUG=
Committed: https://crrev.com/fa4d1f32690b749496963058d53b90ab9e59769a
Cr-Commit-Position: refs/heads/master@{#328783}
Patch Set 1 #
Total comments: 18
Patch Set 2 : Address comments. Fix bot failures. #
Total comments: 25
Patch Set 3 : Remove TextureFormat enum explicit values. #
Total comments: 4
Patch Set 4 : Address reveman's comments. #
Total comments: 4
Messages
Total messages: 43 (11 generated)
dcastagna@chromium.org changed reviewers: + dalecurtis@chromium.org
dalecurtis@chromium.org changed reviewers: + posciak@chromium.org
Seems reasonable to me, but +posciak for his thoughts, since he's done a lot of GPU decoder work. You'll also need more OWNERS for the final review. Seems your bots are legitimately failing, so I'll hold off on approval until there's some more discussion. https://codereview.chromium.org/1117423002/diff/1/media/base/video_frame.h File media/base/video_frame.h (right): https://codereview.chromium.org/1117423002/diff/1/media/base/video_frame.h#ne... media/base/video_frame.h:73: TEXTURE_RGBA = 1, // One RGBA texture. Doesn't android use ARGB or something?
reveman@chromium.org changed reviewers: + reveman@chromium.org
https://codereview.chromium.org/1117423002/diff/1/media/base/video_frame.cc File media/base/video_frame.cc (right): https://codereview.chromium.org/1117423002/diff/1/media/base/video_frame.cc#n... media/base/video_frame.cc:253: mailbox_holders[0] = mailbox_holder; s/0/kARGBPlan/ https://codereview.chromium.org/1117423002/diff/1/media/base/video_frame.h File media/base/video_frame.h (right): https://codereview.chromium.org/1117423002/diff/1/media/base/video_frame.h#ne... media/base/video_frame.h:70: // holders. hm, this defines the subsampling factor and internal format as needed to use these textures in a shader. The actual pixel format does not need to be known. https://codereview.chromium.org/1117423002/diff/1/media/base/video_frame.h#ne... media/base/video_frame.h:72: TEXTURE_NONE = 0, // This frame doesn't contain any native texture. I think the code would be cleaner if this enum contained only valid values and texture_format_ field should be ignored if format is not NATIVE_TEXTURE. e.g. the IsValidConfig change goes away. https://codereview.chromium.org/1117423002/diff/1/media/base/video_frame.h#ne... media/base/video_frame.h:73: TEXTURE_RGBA = 1, // One RGBA texture. On 2015/05/02 at 18:21:33, DaleCurtis wrote: > Doesn't android use ARGB or something? I think the idea is that this just describes the internal format as far as a shader needs to know. TEXTURE_RGBA, means it's one texture with red/green/blue/alpha channels but the pixel layout is unknown. https://codereview.chromium.org/1117423002/diff/1/media/base/video_frame.h#ne... media/base/video_frame.h:74: TEXTURE_YUV_R8R8R8 = 2, // Three R8 textures one per YUV channel. s/YUV_R8R8R8/YUV_420/ as that would make it clear the UV planes have a subsampling factor of 2 and the actual resolution (R8R8R8) of each plane should not be useful at this level. https://codereview.chromium.org/1117423002/diff/1/media/base/video_frame.h#ne... media/base/video_frame.h:123: static scoped_refptr<VideoFrame> WrapYUVNativeTextures( WrapYUV420NativeTextures? Or pass the texture format as a parameter. Or why don't we just change the WrapNativeTexture function above into a "WrapNativeTextures" function that take an array of mailbox holders and a texture format parameter? https://codereview.chromium.org/1117423002/diff/1/media/base/video_frame.h#ne... media/base/video_frame.h:376: const gpu::MailboxHolder mailbox_holders[kMaxPlanes], Pass by reference instead of value. https://codereview.chromium.org/1117423002/diff/1/media/base/video_frame.h#ne... media/base/video_frame.h:387: // Format of the native textures associated with this frames. nit: s/frames/frame/
https://codereview.chromium.org/1117423002/diff/1/media/base/video_frame.cc File media/base/video_frame.cc (right): https://codereview.chromium.org/1117423002/diff/1/media/base/video_frame.cc#n... media/base/video_frame.cc:253: mailbox_holders[0] = mailbox_holder; On 2015/05/03 at 16:13:27, reveman wrote: > s/0/kARGBPlan/ Done. https://codereview.chromium.org/1117423002/diff/1/media/base/video_frame.h File media/base/video_frame.h (right): https://codereview.chromium.org/1117423002/diff/1/media/base/video_frame.h#ne... media/base/video_frame.h:70: // holders. On 2015/05/03 at 16:13:27, reveman wrote: > hm, this defines the subsampling factor and internal format as needed to use these textures in a shader. The actual pixel format does not need to be known. Changed the comment. https://codereview.chromium.org/1117423002/diff/1/media/base/video_frame.h#ne... media/base/video_frame.h:72: TEXTURE_NONE = 0, // This frame doesn't contain any native texture. On 2015/05/03 at 16:13:27, reveman wrote: > I think the code would be cleaner if this enum contained only valid values and texture_format_ field should be ignored if format is not NATIVE_TEXTURE. e.g. the IsValidConfig change goes away. Done. https://codereview.chromium.org/1117423002/diff/1/media/base/video_frame.h#ne... media/base/video_frame.h:73: TEXTURE_RGBA = 1, // One RGBA texture. On 2015/05/03 at 16:13:27, reveman wrote: > On 2015/05/02 at 18:21:33, DaleCurtis wrote: > > Doesn't android use ARGB or something? > > I think the idea is that this just describes the internal format as far as a shader needs to know. TEXTURE_RGBA, means it's one texture with red/green/blue/alpha channels but the pixel layout is unknown. Agreed. https://codereview.chromium.org/1117423002/diff/1/media/base/video_frame.h#ne... media/base/video_frame.h:74: TEXTURE_YUV_R8R8R8 = 2, // Three R8 textures one per YUV channel. On 2015/05/03 at 16:13:27, reveman wrote: > s/YUV_R8R8R8/YUV_420/ as that would make it clear the UV planes have a subsampling factor of 2 and the actual resolution (R8R8R8) of each plane should not be useful at this level. Done. https://codereview.chromium.org/1117423002/diff/1/media/base/video_frame.h#ne... media/base/video_frame.h:123: static scoped_refptr<VideoFrame> WrapYUVNativeTextures( On 2015/05/03 at 16:13:27, reveman wrote: > WrapYUV420NativeTextures? Or pass the texture format as a parameter. Or why don't we just change the WrapNativeTexture function above into a "WrapNativeTextures" function that take an array of mailbox holders and a texture format parameter? Renamed to WrapYUV420NativeTextures. I decided not to change WrapNativeTextures to take an array of mailboxes and a texture format since all the other wrap functions seem to be for a specific video format. The videoframe format is never passed from the outside but always determined by one of this wrap function, so keeping WrapYUV420NativeTextures seems to fit better with the rest of the class. https://codereview.chromium.org/1117423002/diff/1/media/base/video_frame.h#ne... media/base/video_frame.h:376: const gpu::MailboxHolder mailbox_holders[kMaxPlanes], On 2015/05/03 at 16:13:27, reveman wrote: > Pass by reference instead of value. Done. git cl format seems to be confused by the syntax though. https://codereview.chromium.org/1117423002/diff/1/media/base/video_frame.h#ne... media/base/video_frame.h:387: // Format of the native textures associated with this frames. On 2015/05/03 at 16:13:27, reveman wrote: > nit: s/frames/frame/ Done.
https://codereview.chromium.org/1117423002/diff/1/media/base/video_frame.h File media/base/video_frame.h (right): https://codereview.chromium.org/1117423002/diff/1/media/base/video_frame.h#ne... media/base/video_frame.h:72: TEXTURE_NONE = 0, // This frame doesn't contain any native texture. On 2015/05/04 15:00:15, Daniele Castagna wrote: > On 2015/05/03 at 16:13:27, reveman wrote: > > I think the code would be cleaner if this enum contained only valid values and > texture_format_ field should be ignored if format is not NATIVE_TEXTURE. e.g. > the IsValidConfig change goes away. > > Done. Hmm, I think that's fine so long as we still initialize it to RGBA or something. Otherwise I prefer the DCHECK_NE(NONE in places to avoid waiting for valgrind to catch an uninitialized value.
https://codereview.chromium.org/1117423002/diff/20001/media/base/video_frame.cc File media/base/video_frame.cc (right): https://codereview.chromium.org/1117423002/diff/20001/media/base/video_frame.... media/base/video_frame.cc:199: natural_size.height() > limits::kMaxDimension) I think you should keep the checks here. https://codereview.chromium.org/1117423002/diff/20001/media/base/video_frame.... media/base/video_frame.cc:561: return 3; Should be 2 I think. But 1 if you switch to 0,1 scale. https://codereview.chromium.org/1117423002/diff/20001/media/base/video_frame.h File media/base/video_frame.h (right): https://codereview.chromium.org/1117423002/diff/20001/media/base/video_frame.... media/base/video_frame.h:72: TEXTURE_RGBA = 1, // One RGBA texture. Remove =1, =2 ?
On 2015/05/04 at 16:00:15, dalecurtis wrote: > https://codereview.chromium.org/1117423002/diff/1/media/base/video_frame.h > File media/base/video_frame.h (right): > > https://codereview.chromium.org/1117423002/diff/1/media/base/video_frame.h#ne... > media/base/video_frame.h:72: TEXTURE_NONE = 0, // This frame doesn't contain any native texture. > On 2015/05/04 15:00:15, Daniele Castagna wrote: > > On 2015/05/03 at 16:13:27, reveman wrote: > > > I think the code would be cleaner if this enum contained only valid values and > > texture_format_ field should be ignored if format is not NATIVE_TEXTURE. e.g. > > the IsValidConfig change goes away. > > > > Done. > > Hmm, I think that's fine so long as we still initialize it to RGBA or something. Otherwise I prefer the DCHECK_NE(NONE in places to avoid waiting for valgrind to catch an uninitialized value. Initialized it to RGBA. VideoFrame::texture_format_ is const, so it has to be initialized.
mcasas@chromium.org changed reviewers: + mcasas@chromium.org
Some drive-by on the practicalities. https://codereview.chromium.org/1117423002/diff/20001/media/base/video_frame.cc File media/base/video_frame.cc (right): https://codereview.chromium.org/1117423002/diff/20001/media/base/video_frame.... media/base/video_frame.cc:146: mailboxes, TEXTURE_RGBA, timestamp, false)); Although this is more Chromium-style, I'd rather leave it as it was so it's easier to see the added parameter. https://codereview.chromium.org/1117423002/diff/20001/media/base/video_frame.... media/base/video_frame.cc:253: mailbox_holders, TEXTURE_RGBA, timestamp, false)); Idem, please align parameters one per line (again, media style locally overriding Cr style). https://codereview.chromium.org/1117423002/diff/20001/media/base/video_frame.h File media/base/video_frame.h (right): https://codereview.chromium.org/1117423002/diff/20001/media/base/video_frame.... media/base/video_frame.h:75: }; I see here that we're creating a second layer of types in VideoFrame, the first being for memory backed frames and a secondary for Texture-backed one. This forces you to add accessors for this second layer. What about duplicating line 59 NATIVE_TEXTURE into e.g. SINGLE_TEXTURE(_RGBA) THREE_TEXTURES(_YUV) (and we could add TWO_TEXTURES for e.g. NV21 down the road easily, if ever). (Careful with the UMA histograms though.) This will allow for merging WrapNativeTexture() and WrapY420NativeTextures() into a single method accepting as many textures as the planarity indicates, and we'll treat the multiple textures as planes which makes sense semantic-wise IMHO. (btw what's with the "NATIVE"? Are there non-"NATIVE" textures? Consider removing it). https://codereview.chromium.org/1117423002/diff/20001/media/base/video_frame.... media/base/video_frame.h:238: static size_t NumTextures(TextureFormat texture_format); Isn't NumTextures() going to be the same as NumPlanes() always? https://codereview.chromium.org/1117423002/diff/20001/media/base/video_frame.... media/base/video_frame.h:374: const gpu::MailboxHolder(&mailbox_holders)[kMaxPlanes], This is an unexpected change to me. Why change the ownership semantics of the MailboxHolder just because we change their multiplicity?
hubbe@chromium.org changed reviewers: + hubbe@chromium.org
https://codereview.chromium.org/1117423002/diff/20001/media/base/video_frame.h File media/base/video_frame.h (right): https://codereview.chromium.org/1117423002/diff/20001/media/base/video_frame.... media/base/video_frame.h:73: TEXTURE_YUV_420 = 2, // Three R8 textures one per YUV channel. FYI: In the tab capture pipeline we use RGBA instead of R8 textures to hold YUV data. (Each RGBA pixel holds four consecutive R8 values.) This is because GLES doesn't always support R8 textures, but RGBA textures are guaranteed to be there. Not sure if/how this applies to your use case, but I wanted to let you know. Also, tab capture might want to use this facility in the future, so it would be nice for us if it was compatible.
https://codereview.chromium.org/1117423002/diff/20001/media/base/video_frame.cc File media/base/video_frame.cc (right): https://codereview.chromium.org/1117423002/diff/20001/media/base/video_frame.... media/base/video_frame.cc:253: mailbox_holders, TEXTURE_RGBA, timestamp, false)); On 2015/05/04 16:23:41, mcasas wrote: > Idem, please align parameters one per line (again, > media style locally overriding Cr style). I'm fine with git cl format on this :)
https://codereview.chromium.org/1117423002/diff/20001/media/base/video_frame.cc File media/base/video_frame.cc (right): https://codereview.chromium.org/1117423002/diff/20001/media/base/video_frame.... media/base/video_frame.cc:146: mailboxes, TEXTURE_RGBA, timestamp, false)); On 2015/05/04 at 16:23:41, mcasas wrote: > Although this is more Chromium-style, I'd rather > leave it as it was so it's easier to see the > added parameter. This was done by "git cl format". dalecurtis@ seems to be ok with this. https://codereview.chromium.org/1117423002/diff/20001/media/base/video_frame.... media/base/video_frame.cc:199: natural_size.height() > limits::kMaxDimension) On 2015/05/04 at 16:12:06, DaleCurtis wrote: > I think you should keep the checks here. What can we check now if texture_format is always valid? https://codereview.chromium.org/1117423002/diff/20001/media/base/video_frame.... media/base/video_frame.cc:253: mailbox_holders, TEXTURE_RGBA, timestamp, false)); On 2015/05/04 at 16:31:30, DaleCurtis wrote: > On 2015/05/04 16:23:41, mcasas wrote: > > Idem, please align parameters one per line (again, > > media style locally overriding Cr style). > > I'm fine with git cl format on this :) Tnx, I find it really time wasting to have to run "git cl format" and then have to go back to undo some changes. https://codereview.chromium.org/1117423002/diff/20001/media/base/video_frame.... media/base/video_frame.cc:561: return 3; On 2015/05/04 at 16:12:06, DaleCurtis wrote: > Should be 2 I think. But 1 if you switch to 0,1 scale. I'm afraid I don't understand what you mean. This should return the number of valid mailboxes in this VideoFrame. If VideoFrame is created with WrapYUV420NativeTextures there should be 3 valid mailboxes, one for every channel. Why are you suggesting 2? https://codereview.chromium.org/1117423002/diff/20001/media/base/video_frame.h File media/base/video_frame.h (right): https://codereview.chromium.org/1117423002/diff/20001/media/base/video_frame.... media/base/video_frame.h:72: TEXTURE_RGBA = 1, // One RGBA texture. On 2015/05/04 at 16:12:06, DaleCurtis wrote: > Remove =1, =2 ? All the other enums in the class seem to explicitly specify the values. Do you prefer to handle this differently? https://codereview.chromium.org/1117423002/diff/20001/media/base/video_frame.... media/base/video_frame.h:73: TEXTURE_YUV_420 = 2, // Three R8 textures one per YUV channel. On 2015/05/04 at 16:26:31, hubbe wrote: > FYI: In the tab capture pipeline we use RGBA instead of R8 textures to hold YUV data. (Each RGBA pixel holds four consecutive R8 values.) This is because GLES doesn't always support R8 textures, but RGBA textures are guaranteed to be there. Not sure if/how this applies to your use case, but I wanted to let you know. Also, tab capture might want to use this facility in the future, so it would be nice for us if it was compatible. Got it, we should add another texture format after this patch lands. Just for curiousity's sake, where is the shader that draws those textures? https://codereview.chromium.org/1117423002/diff/20001/media/base/video_frame.... media/base/video_frame.h:75: }; On 2015/05/04 at 16:23:42, mcasas wrote: > I see here that we're creating a second layer of > types in VideoFrame, the first being for memory > backed frames and a secondary for Texture-backed > one. This forces you to add accessors for this > second layer. What about duplicating line > 59 NATIVE_TEXTURE into e.g. > SINGLE_TEXTURE(_RGBA) > THREE_TEXTURES(_YUV) > (and we could add TWO_TEXTURES for e.g. NV21 > down the road easily, if ever). > > (Careful with the UMA histograms though.) > > This will allow for merging WrapNativeTexture() > and WrapY420NativeTextures() into a single > method accepting as many textures as the > planarity indicates, and we'll treat the > multiple textures as planes which makes > sense semantic-wise IMHO. > > (btw what's with the "NATIVE"? Are there > non-"NATIVE" textures? Consider removing it). We thought about the solution you're suggesting too. I don't have any strong preference about this one or the other. After a quick chat with dalecurtis@ he suggested to go with the approach in this patch though. dalecurtis@, do you mind explaining why you seemed to prefer to add another enum for the texture_format? https://codereview.chromium.org/1117423002/diff/20001/media/base/video_frame.... media/base/video_frame.h:238: static size_t NumTextures(TextureFormat texture_format); On 2015/05/04 at 16:23:41, mcasas wrote: > Isn't NumTextures() going to be the same as NumPlanes() > always? NumPlanes is generally used to determine how data_ can be accessed, but in the case of NATIVE_TEXTURE data_ is not set. NumPlanes returns 0 when video_format is NATIVE_TEXTURE since there is no data. NumTextures should be used to determine how many valid mailboxes there are. In case of non NATIVE_TEXTURE, NumTextures should be 0 since there are no valid mailboxes. https://codereview.chromium.org/1117423002/diff/20001/media/base/video_frame.... media/base/video_frame.h:374: const gpu::MailboxHolder(&mailbox_holders)[kMaxPlanes], On 2015/05/04 at 16:23:41, mcasas wrote: > This is an unexpected change to me. Why > change the ownership semantics of the > MailboxHolder just because we change their > multiplicity? MailboxHolder contains two integers and a Mailbox. Mailbox was meant to be passed around by value and it basically contains just a string. I didn't see any point in passing around ownership of a string and two integers in the first place. Getting rid of the scoped_ptr makes the interface cleaner for the new added WrapNativeYUV420Textures and for the old WrapNativeTexture (e.g: the mailboxholder copy in TextureWrapHelper::OnIncomingCapturedGpuMemoryBuffer).
https://codereview.chromium.org/1117423002/diff/20001/media/base/video_frame.cc File media/base/video_frame.cc (right): https://codereview.chromium.org/1117423002/diff/20001/media/base/video_frame.... media/base/video_frame.cc:199: natural_size.height() > limits::kMaxDimension) On 2015/05/04 17:59:59, Daniele Castagna wrote: > On 2015/05/04 at 16:12:06, DaleCurtis wrote: > > I think you should keep the checks here. > > What can we check now if texture_format is always valid? IsValid() checks like this are sometimes used to validate data as it crosses an IPC boundary; in those cases a malicious renderer could manipulate the value. I couldn't remember if we pass VideoFrames as is across the boundary, but it looks like we don't, so it seems a moot point https://codereview.chromium.org/1117423002/diff/20001/media/base/video_frame.... media/base/video_frame.cc:561: return 3; On 2015/05/04 17:59:59, Daniele Castagna wrote: > On 2015/05/04 at 16:12:06, DaleCurtis wrote: > > Should be 2 I think. But 1 if you switch to 0,1 scale. > > I'm afraid I don't understand what you mean. > This should return the number of valid mailboxes in this VideoFrame. > If VideoFrame is created with WrapYUV420NativeTextures there should be 3 valid > mailboxes, one for every channel. > Why are you suggesting 2? Derp, I though this was converting the enum value. https://codereview.chromium.org/1117423002/diff/20001/media/base/video_frame.h File media/base/video_frame.h (right): https://codereview.chromium.org/1117423002/diff/20001/media/base/video_frame.... media/base/video_frame.h:72: TEXTURE_RGBA = 1, // One RGBA texture. On 2015/05/04 17:59:59, Daniele Castagna wrote: > On 2015/05/04 at 16:12:06, DaleCurtis wrote: > > Remove =1, =2 ? > > > All the other enums in the class seem to explicitly specify the values. Do you > prefer to handle this differently? That's because they're used in UMA calculations. https://codereview.chromium.org/1117423002/diff/20001/media/base/video_frame.... media/base/video_frame.h:75: }; On 2015/05/04 17:59:59, Daniele Castagna wrote: > On 2015/05/04 at 16:23:42, mcasas wrote: > > I see here that we're creating a second layer of > > types in VideoFrame, the first being for memory > > backed frames and a secondary for Texture-backed > > one. This forces you to add accessors for this > > second layer. What about duplicating line > > 59 NATIVE_TEXTURE into e.g. > > SINGLE_TEXTURE(_RGBA) > > THREE_TEXTURES(_YUV) > > (and we could add TWO_TEXTURES for e.g. NV21 > > down the road easily, if ever). > > > > (Careful with the UMA histograms though.) > > > > This will allow for merging WrapNativeTexture() > > and WrapY420NativeTextures() into a single > > method accepting as many textures as the > > planarity indicates, and we'll treat the > > multiple textures as planes which makes > > sense semantic-wise IMHO. > > > > (btw what's with the "NATIVE"? Are there > > non-"NATIVE" textures? Consider removing it). > > We thought about the solution you're suggesting too. I don't have any strong > preference about this one or the other. > After a quick chat with dalecurtis@ he suggested to go with the approach in this > patch though. > > dalecurtis@, do you mind explaining why you seemed to prefer to add another enum > for the texture_format? I don't have a strong preference either way. NATIVE_TEXTURE explicitly says it's pixel format agnostic though, so unless we want to rewrite what NATIVE_TEXTURE means everywhere with say NATIVE_TEXT_ARGB, etc, this approach seems better.
https://codereview.chromium.org/1117423002/diff/20001/media/base/video_frame.h File media/base/video_frame.h (right): https://codereview.chromium.org/1117423002/diff/20001/media/base/video_frame.... media/base/video_frame.h:73: TEXTURE_YUV_420 = 2, // Three R8 textures one per YUV channel. On 2015/05/04 17:59:59, Daniele Castagna wrote: > On 2015/05/04 at 16:26:31, hubbe wrote: > > FYI: In the tab capture pipeline we use RGBA instead of R8 textures to hold > YUV data. (Each RGBA pixel holds four consecutive R8 values.) This is because > GLES doesn't always support R8 textures, but RGBA textures are guaranteed to be > there. Not sure if/how this applies to your use case, but I wanted to let you > know. Also, tab capture might want to use this facility in the future, so it > would be nice for us if it was compatible. > > Got it, we should add another texture format after this patch lands. > > Just for curiousity's sake, where is the shader that draws those textures? If by "draws", you mean "displays on the screen", then the answer is that we don't have any such shaders. In tab capture, we convert textures to YUV on the gpu to read them back into cpu memory more efficiently. The shaders that generate generate these textures are here: https://code.google.com/p/chromium/codesearch#chromium/src/content/common/gpu... The end goal in tab capture is to video-encode the textures. Normally we do this in software with a vp8 encoder. On platforms where video encoding can happen on the gpu, we'll want to not do the readback, and that is where we might want to use this representation. (Depending on what the video codec expects as input of course.)
https://codereview.chromium.org/1117423002/diff/20001/media/base/video_frame.h File media/base/video_frame.h (right): https://codereview.chromium.org/1117423002/diff/20001/media/base/video_frame.... media/base/video_frame.h:72: TEXTURE_RGBA = 1, // One RGBA texture. On 2015/05/04 at 18:12:54, DaleCurtis wrote: > On 2015/05/04 17:59:59, Daniele Castagna wrote: > > On 2015/05/04 at 16:12:06, DaleCurtis wrote: > > > Remove =1, =2 ? > > > > > > All the other enums in the class seem to explicitly specify the values. Do you > > prefer to handle this differently? > > That's because they're used in UMA calculations. Got it. Done.
lgtm https://codereview.chromium.org/1117423002/diff/40001/media/base/video_frame.h File media/base/video_frame.h (right): https://codereview.chromium.org/1117423002/diff/40001/media/base/video_frame.... media/base/video_frame.h:73: TEXTURE_YUV_420, // Three R8 textures one per YUV channel. nit: s/R8/RED/? As the actual precision of each texture is not necessarily known. And if you're describing the formats here then it's probably a good idea to mention that it contains 2x2 subsampled UV planes as that's information that is needed to render this correctly. https://codereview.chromium.org/1117423002/diff/40001/media/base/video_frame.... media/base/video_frame.h:74: TEXTURE_FORMAT_MAX = TEXTURE_YUV_420, nit: is this used?
lgtm
Anyone think we're quickly approaching the point where media::VideoFrame needs to become a base class and then specialized by subclassing according to the type of memory backing? For example, I can imagine these three classes OTOH: media::HeapAllocatedVideoFrame (good 'ol heap storage), media::SharedMemoryVideoFrame (for passing between processes), and media::MailboxVideoFrame (lives on GPU)?
I think that's a great idea if possible, the vast majority of consumers don't care anything about how these frames are backed.
On 2015/05/05 at 05:49:46, dalecurtis wrote: > I think that's a great idea if possible, the vast majority of consumers don't care anything about how these frames are backed. I agree that VideoFrame needs to be modularized but I'd still suggest to go ahead and land this patch if we want to try to have GpuMemoryBuffer backed VideoFrames before the branch cut. Please let me know if you're strongly against this.
https://codereview.chromium.org/1117423002/diff/40001/media/base/video_frame.h File media/base/video_frame.h (right): https://codereview.chromium.org/1117423002/diff/40001/media/base/video_frame.... media/base/video_frame.h:73: TEXTURE_YUV_420, // Three R8 textures one per YUV channel. On 2015/05/04 at 21:49:00, reveman wrote: > nit: s/R8/RED/? As the actual precision of each texture is not necessarily known. And if you're describing the formats here then it's probably a good idea to mention that it contains 2x2 subsampled UV planes as that's information that is needed to render this correctly. Done. https://codereview.chromium.org/1117423002/diff/40001/media/base/video_frame.... media/base/video_frame.h:74: TEXTURE_FORMAT_MAX = TEXTURE_YUV_420, On 2015/05/04 at 21:49:00, reveman wrote: > nit: is this used? Done.
dcastagna@chromium.org changed reviewers: + piman@chromium.org
+piman for cc/ owner's approval.
dcastagna@chromium.org changed reviewers: - piman@chromium.org
-piman, since reveman's is an owner of cc. :/
piman@chromium.org changed reviewers: + piman@chromium.org
lgtm https://codereview.chromium.org/1117423002/diff/60001/media/base/video_frame.h File media/base/video_frame.h (right): https://codereview.chromium.org/1117423002/diff/60001/media/base/video_frame.... media/base/video_frame.h:412: gpu::MailboxHolder mailbox_holders_[kMaxPlanes]; drive-by: FYI, gpu::Mailbox is 64 bytes, so MailboxHolder is 72 bytes. You just added 280 bytes to all VideoFrames. Not sure if it's a concern.
posciak@chromium.org changed reviewers: + marcheu@chromium.org
+marcheu FYI
https://codereview.chromium.org/1117423002/diff/60001/media/base/video_frame.h File media/base/video_frame.h (right): https://codereview.chromium.org/1117423002/diff/60001/media/base/video_frame.... media/base/video_frame.h:412: gpu::MailboxHolder mailbox_holders_[kMaxPlanes]; On 2015/05/06 at 01:08:09, piman (Very slow to review) wrote: > drive-by: FYI, gpu::Mailbox is 64 bytes, so MailboxHolder is 72 bytes. You just added 280 bytes to all VideoFrames. Not sure if it's a concern. I think it's fine. We don't have many VideoFrames allocated at the same time, in my tests there were about 7 (let's say 14 since we've the wrapping thing going on), and usually they have attached the frame data that is much bigger than 280 bytes.
Sorry for the delay, holidays here this week. In general, I'm ok with this, however this is also why we already have dmabufs in VideoFrame for CrOS, i.e. VF backed by (multiple) (native) memory buffer(s). Ideally it would be great to get rid of them if we could use textures as the abstraction for them as well. The key would then be to be able to get to the underlying GLImage from texture and then to dmabufs. If we could have that path, this would work great for CrOS as well. For example, when the capturer produces raw frames into dmabuf-backed buffers, that will be passed to webrtc in renderer and back to VideoEncodeAccelerator in GPU process for encoding. We need to be able to extract the dmabufs from the VF in VEA to be able to encode from them. https://codereview.chromium.org/1117423002/diff/60001/media/base/video_frame.h File media/base/video_frame.h (right): https://codereview.chromium.org/1117423002/diff/60001/media/base/video_frame.... media/base/video_frame.h:71: enum TextureFormat { Would it perhaps be possible to use an existing set of defines/enums for this? We have multiple format enums already in various structures. I would be great if we could have less of these if that would be possible. A good set could be fourccs perhaps? They are more or less universally used and there are defines for 2-plane and 3-plane formats as well, e.g. https://code.google.com/p/chromium/codesearch#chromium/usr/include/drm/drm_fo.... We already use them for accelerated video in Chrome for native buffers as well.
Also, second usage would be to get to dmabufs for rendering purposes, for example if we use overlays for video decode, we'd need the GPU process to be able to extract dmabufs to use them in displaying the decoded video frames.
Per offline discussion, let's consider the above scenarios a bit later. I'm ok getting this in in this shape. lgtm % my question/suggestion about the format enum.
https://codereview.chromium.org/1117423002/diff/60001/media/base/video_frame.h File media/base/video_frame.h (right): https://codereview.chromium.org/1117423002/diff/60001/media/base/video_frame.... media/base/video_frame.h:71: enum TextureFormat { On 2015/05/06 at 02:14:29, Pawel Osciak wrote: > Would it perhaps be possible to use an existing set of defines/enums for this? We have multiple format enums already in various structures. I would be great if we could have less of these if that would be possible. > > A good set could be fourccs perhaps? They are more or less universally used and there are defines for 2-plane and 3-plane formats as well, e.g. https://code.google.com/p/chromium/codesearch#chromium/usr/include/drm/drm_fo.... We already use them for accelerated video in Chrome for native buffers as well. I'm afraid the existing defines/enums are not a good fit for what we want here. We want to use this enum to let the compositor determine how many textures we have and which shader needs to be used to correctly display the frame. The DRM_FORMAT_YVU420 for example could map to at least two TextureFormat enums: - One texture backed by a YUV GpuMemoryBuffer. - Three textures backed by three RED GpuMemoryBuffers.
The CQ bit was checked by dcastagna@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from reveman@chromium.org, dalecurtis@chromium.org Link to the patchset: https://codereview.chromium.org/1117423002/#ps60001 (title: "Address reveman's comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1117423002/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/fa4d1f32690b749496963058d53b90ab9e59769a Cr-Commit-Position: refs/heads/master@{#328783} |
