Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(11)

Issue 1874733002: media: split GpuMemoryBufferVideoFramePool into GpuMemoryBufferVideoFrameCopier/Pool

Created:
4 years, 8 months ago by dshwang
Modified:
4 years, 7 months ago
CC:
chromium-reviews, darin-cc_chromium.org, feature-media-reviews_chromium.org, jam, mcasas+watch_chromium.org, mkwst+moarreviews-renderer_chromium.org, mlamouri+watch-content_chromium.org, posciak+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

media: split GpuMemoryBufferVideoFramePool into GpuMemoryBufferVideoFrameCopier/Pool Current GpuMemoryBufferVideoFramePool has only one API, which copies software VideoFrame to GpuMemoryBuffer VideoFrame. However, GpuMemoryBufferVideoFramePool can have new API to provide new VideoFrame managed by internal pool like VideoFramePool. If GpuMemoryBufferVideoFramePool provides new GMB VideoFrame, each decoder (e.g. ffmpeg, vpx) can decode video frame on the GMB VideoFrame. So split GpuMemoryBufferVideoFramePool into GpuMemoryBufferVideoFrameCopier/Pool. Now GpuMemoryBufferVideoFramePool is very similar to VideoFramePool except for producing a GMB VideoFrame. GpuMemoryBufferVideoFrameCopier has same API to old GpuMemoryBufferVideoFramePool. TEST=media_unittests --gtest_filter=GpuMemoryBufferVideoFrame(Copier|Pool)* BUG=601788, 590358

Patch Set 1 #

Patch Set 2 : build fix #

Patch Set 3 : add GpuMemoryBufferVideoFramePoolTest #

Unified diffs Side-by-side diffs Delta from patch set Stats (+1173 lines, -923 lines) Patch
M content/renderer/media/media_stream_video_renderer_sink.h View 1 2 3 chunks +4 lines, -3 lines 0 comments Download
M content/renderer/media/media_stream_video_renderer_sink.cc View 3 chunks +10 lines, -10 lines 0 comments Download
M content/renderer/media/media_stream_video_renderer_sink_unittest.cc View 2 chunks +2 lines, -2 lines 0 comments Download
M media/BUILD.gn View 1 2 3 chunks +5 lines, -2 lines 0 comments Download
M media/media.gyp View 1 2 3 chunks +5 lines, -2 lines 0 comments Download
A + media/renderers/mock_gpu_memory_buffer_video_frame_copier.h View 1 chunk +5 lines, -3 lines 0 comments Download
A + media/renderers/mock_gpu_memory_buffer_video_frame_copier.cc View 1 chunk +2 lines, -2 lines 0 comments Download
D media/renderers/mock_gpu_memory_buffer_video_frame_pool.h View 1 chunk +0 lines, -26 lines 0 comments Download
D media/renderers/mock_gpu_memory_buffer_video_frame_pool.cc View 1 chunk +0 lines, -17 lines 0 comments Download
M media/renderers/video_renderer_impl.h View 3 chunks +3 lines, -3 lines 0 comments Download
M media/renderers/video_renderer_impl.cc View 6 chunks +7 lines, -7 lines 0 comments Download
M media/renderers/video_renderer_impl_unittest.cc View 2 chunks +4 lines, -4 lines 0 comments Download
A + media/video/gpu_memory_buffer_video_frame_copier.h View 3 chunks +12 lines, -17 lines 0 comments Download
A media/video/gpu_memory_buffer_video_frame_copier.cc View 1 chunk +393 lines, -0 lines 0 comments Download
A + media/video/gpu_memory_buffer_video_frame_copier_unittest.cc View 1 2 16 chunks +32 lines, -32 lines 0 comments Download
M media/video/gpu_memory_buffer_video_frame_pool.h View 1 2 3 chunks +46 lines, -19 lines 0 comments Download
M media/video/gpu_memory_buffer_video_frame_pool.cc View 1 2 3 chunks +490 lines, -645 lines 0 comments Download
D media/video/gpu_memory_buffer_video_frame_pool_unittest.cc View 1 2 3 chunks +153 lines, -129 lines 0 comments Download

Messages

Total messages: 36 (18 generated)
dshwang
Daniele, could you review? I extract this CL from https://codereview.chromium.org/1869303004/ to make it easy to ...
4 years, 8 months ago (2016-04-08 15:08:23 UTC) #4
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1874733002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1874733002/1
4 years, 8 months ago (2016-04-08 15:15:34 UTC) #6
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1874733002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1874733002/40001
4 years, 8 months ago (2016-04-08 15:26:12 UTC) #9
commit-bot: I haz the power
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_ng/builds/208094)
4 years, 8 months ago (2016-04-08 16:56:39 UTC) #11
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1874733002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1874733002/60001
4 years, 8 months ago (2016-04-11 15:42:46 UTC) #14
commit-bot: I haz the power
Dry run: Try jobs failed on following builders: ios_rel_device_gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_gn/builds/17071) ios_rel_device_ninja on ...
4 years, 8 months ago (2016-04-11 15:44:47 UTC) #17
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1874733002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1874733002/80001
4 years, 8 months ago (2016-04-11 15:48:53 UTC) #20
commit-bot: I haz the power
Dry run: Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/166944)
4 years, 8 months ago (2016-04-11 15:58:01 UTC) #22
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1874733002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1874733002/100001
4 years, 8 months ago (2016-04-11 16:08:06 UTC) #25
commit-bot: I haz the power
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_ng/builds/208879)
4 years, 8 months ago (2016-04-11 18:07:04 UTC) #27
dshwang
Daniele, Dale, PTAL?
4 years, 7 months ago (2016-05-04 10:02:36 UTC) #28
DaleCurtis
I'm not familiar with Daniele's more recent work, so I defer to him until this ...
4 years, 7 months ago (2016-05-09 18:54:37 UTC) #29
Daniele Castagna
Sorry for the delay, I'm currently traveling. I'm going to be back in the office ...
4 years, 7 months ago (2016-05-11 03:03:17 UTC) #30
dshwang
On 2016/05/11 03:03:17, Daniele Castagna wrote: > Sorry for the delay, I'm currently traveling. > ...
4 years, 7 months ago (2016-05-11 07:30:45 UTC) #31
Daniele Castagna
A few questions/observations from a high level point of view. You don't explain in the ...
4 years, 7 months ago (2016-05-18 04:30:07 UTC) #32
dshwang
On 2016/05/18 04:30:07, Daniele Castagna wrote: > A few questions/observations from a high level point ...
4 years, 7 months ago (2016-05-18 18:36:10 UTC) #34
Daniele Castagna
On 2016/05/18 at 18:36:10, dongseong.hwang wrote: > On 2016/05/18 04:30:07, Daniele Castagna wrote: > > ...
4 years, 7 months ago (2016-05-19 18:08:04 UTC) #35
dshwang
4 years, 7 months ago (2016-05-19 18:28:39 UTC) #36
On 2016/05/19 18:08:04, Daniele Castagna wrote:
> You said:
> "I tried to do with ffmpeg, but I could not implement promising prototype
> because
> - I could not know whether the frame is key frame and dependent frame
> - ffmpeg release bunch of frames together (which means I have to copy all the
> bunch of frames)"
> 
> Do you mind uploading a CL with the prototype?

The prototype is lots of LOG(ERROR) based on
https://codereview.chromium.org/1871383003/
When ffmpeg releases a fraem, following callback is called 'static void
ReleaseVideoBufferImpl(void* opaque, uint8_t* data)'
I just compare |data| pointer value between decoding and releasing.

> If ffmpeg releases a frame, it means we don't need to copy it. I don't
> understand why you're saying we need to copy them when they get released.

My explanation seems to be not clear.
As ffmpeg releases a frame too late, we cannot wait it in order to produce new
VideoFrame. So we need to copy them during ffmpeg holding the frame.

Powered by Google App Engine
This is Rietveld 408576698