Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
937643006:
Pepper: add video_encoder example (Closed)
|
Created:
5 years, 10 months ago by llandwerlin-old Modified:
5 years, 6 months ago Reviewers:
bbudge CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@bbudge-ppb-video-encoder-impl Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionPepper: add video_encoder example
BUG=417589
TEST=run ppapi/examples/video_encode/video_encode.html
Committed: https://crrev.com/5f3145d574f2631845867f37a38adec12dab1a7f
Cr-Commit-Position: refs/heads/master@{#318785}
Patch Set 1 #
Total comments: 12
Patch Set 2 : Nits update #
Total comments: 7
Patch Set 3 : Always encode the latest frame #
Total comments: 8
Patch Set 4 : Final nits #Patch Set 5 : Missing GN build files #Patch Set 6 : Fix native compilation on Windows #
Messages
Total messages: 30 (9 generated)
lionel.g.landwerlin@intel.com changed reviewers: + bbudge@chromium.org
Maybe with can get the review going on the example in parallel. Depends on https://codereview.chromium.org/905023005/
https://codereview.chromium.org/937643006/diff/1/ppapi/examples/video_encode/... File ppapi/examples/video_encode/video_encode.cc (right): https://codereview.chromium.org/937643006/diff/1/ppapi/examples/video_encode/... ppapi/examples/video_encode/video_encode.cc:1: // Copyright (c) 2014 The Chromium Authors. All rights reserved. nit: remove (c) and 2015 https://codereview.chromium.org/937643006/diff/1/ppapi/examples/video_encode/... ppapi/examples/video_encode/video_encode.cc:66: // No default to catch unhandled profiles. nit: indent https://codereview.chromium.org/937643006/diff/1/ppapi/examples/video_encode/... ppapi/examples/video_encode/video_encode.cc:79: // No default to catch unhandled accelerations. nit: indent https://codereview.chromium.org/937643006/diff/1/ppapi/examples/video_encode/... ppapi/examples/video_encode/video_encode.cc:169: // nit: remove https://codereview.chromium.org/937643006/diff/1/ppapi/examples/video_encode/... ppapi/examples/video_encode/video_encode.cc:305: } I don't understand why you need to schedule getting a frame from the encoder. Why not make an initial GetVideoFrame request just before you request your first bitstream buffer (in OnInitializedEncoder above) and then request an additional one each time you send a frame to the Encoder? You would need to be able to store one frame from the video track and the encoder, and copy the frame when you had received both. Or am I missing something?
https://codereview.chromium.org/937643006/diff/1/ppapi/examples/video_encode/... File ppapi/examples/video_encode/video_encode.cc (right): https://codereview.chromium.org/937643006/diff/1/ppapi/examples/video_encode/... ppapi/examples/video_encode/video_encode.cc:305: } On 2015/02/18 23:43:24, bbudge wrote: > I don't understand why you need to schedule getting a frame from the encoder. > Why not make an initial GetVideoFrame request just before you request your first > bitstream buffer (in OnInitializedEncoder above) and then request an additional > one each time you send a frame to the Encoder? You would need to be able to > store one frame from the video track and the encoder, and copy the frame when > you had received both. Or am I missing something? There isn't much point to encode as fast at possible. This can lead to generating more frames than what is actually needed. That's why there is this 30fps scheduling.
https://codereview.chromium.org/937643006/diff/1/ppapi/examples/video_encode/... File ppapi/examples/video_encode/video_encode.cc (right): https://codereview.chromium.org/937643006/diff/1/ppapi/examples/video_encode/... ppapi/examples/video_encode/video_encode.cc:1: // Copyright (c) 2014 The Chromium Authors. All rights reserved. On 2015/02/18 23:43:22, bbudge wrote: > nit: remove (c) and 2015 Done. https://codereview.chromium.org/937643006/diff/1/ppapi/examples/video_encode/... ppapi/examples/video_encode/video_encode.cc:66: // No default to catch unhandled profiles. On 2015/02/18 23:43:24, bbudge wrote: > nit: indent Done. https://codereview.chromium.org/937643006/diff/1/ppapi/examples/video_encode/... ppapi/examples/video_encode/video_encode.cc:79: // No default to catch unhandled accelerations. On 2015/02/18 23:43:24, bbudge wrote: > nit: indent Done. https://codereview.chromium.org/937643006/diff/1/ppapi/examples/video_encode/... ppapi/examples/video_encode/video_encode.cc:169: // On 2015/02/18 23:43:24, bbudge wrote: > nit: remove Done.
https://codereview.chromium.org/937643006/diff/1/ppapi/examples/video_encode/... File ppapi/examples/video_encode/video_encode.cc (right): https://codereview.chromium.org/937643006/diff/1/ppapi/examples/video_encode/... ppapi/examples/video_encode/video_encode.cc:305: } On 2015/02/19 00:22:25, llandwerlin wrote: > On 2015/02/18 23:43:24, bbudge wrote: > > I don't understand why you need to schedule getting a frame from the encoder. > > Why not make an initial GetVideoFrame request just before you request your > first > > bitstream buffer (in OnInitializedEncoder above) and then request an > additional > > one each time you send a frame to the Encoder? You would need to be able to > > store one frame from the video track and the encoder, and copy the frame when > > you had received both. Or am I missing something? > > There isn't much point to encode as fast at possible. This can lead to > generating more frames than what is actually needed. That's why there is this > 30fps scheduling. How does it work if the video track is running faster, say 60 fps. Do we drop frames? https://codereview.chromium.org/937643006/diff/20001/ppapi/examples/video_enc... File ppapi/examples/video_encode/video_encode.cc (right): https://codereview.chromium.org/937643006/diff/20001/ppapi/examples/video_enc... ppapi/examples/video_encode/video_encode.cc:245: InitializeEncoder(); Suggestion: inline the function here, so nobody will think it can be called independently of ProbeEncoder. https://codereview.chromium.org/937643006/diff/20001/ppapi/examples/video_enc... File ppapi/examples/video_encode/video_encode.html (right): https://codereview.chromium.org/937643006/diff/20001/ppapi/examples/video_enc... ppapi/examples/video_encode/video_encode.html:4: Copyright 2014 The Chromium Authors. All rights reserved. nit: 2015 https://codereview.chromium.org/937643006/diff/20001/ppapi/examples/video_enc... ppapi/examples/video_encode/video_encode.html:40: navigator.webkitGetUserMedia({audio: false, video: true}, success, failure); nit: line length > 80 https://codereview.chromium.org/937643006/diff/20001/ppapi/examples/video_enc... ppapi/examples/video_encode/video_encode.html:64: saveBlob(new Blob([dataArray], { type: "application/octet-stream" })); On my Mac, the encoder failed (no hardware) and I got a strange file with 0 bytes, a GUID-like file name, and no extension. Is the idea that the file can be played back? If not, it would be nice to be able to see the data on the page, since these examples are sometimes the only way we have to test the proxy. Perhaps a textbox displaying the last buffer encoded?
The example is not displaying the size of the bitstream buffer, giving some feedback on whether the thing is working. https://codereview.chromium.org/937643006/diff/1/ppapi/examples/video_encode/... File ppapi/examples/video_encode/video_encode.cc (right): https://codereview.chromium.org/937643006/diff/1/ppapi/examples/video_encode/... ppapi/examples/video_encode/video_encode.cc:305: } On 2015/02/19 21:54:38, bbudge wrote: > On 2015/02/19 00:22:25, llandwerlin wrote: > > On 2015/02/18 23:43:24, bbudge wrote: > > > I don't understand why you need to schedule getting a frame from the > encoder. > > > Why not make an initial GetVideoFrame request just before you request your > > first > > > bitstream buffer (in OnInitializedEncoder above) and then request an > > additional > > > one each time you send a frame to the Encoder? You would need to be able to > > > store one frame from the video track and the encoder, and copy the frame > when > > > you had received both. Or am I missing something? > > > > There isn't much point to encode as fast at possible. This can lead to > > generating more frames than what is actually needed. That's why there is this > > 30fps scheduling. > > How does it work if the video track is running faster, say 60 fps. Do we drop > frames? In the case of MediaStreamVideoTrack, it will actually drop frames for us : https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/p... I updated the example to extract frame as quick as possible and only encode the latest one. https://codereview.chromium.org/937643006/diff/20001/ppapi/examples/video_enc... File ppapi/examples/video_encode/video_encode.cc (right): https://codereview.chromium.org/937643006/diff/20001/ppapi/examples/video_enc... ppapi/examples/video_encode/video_encode.cc:245: InitializeEncoder(); On 2015/02/19 21:54:38, bbudge wrote: > Suggestion: inline the function here, so nobody will think it can be called > independently of ProbeEncoder. Done. https://codereview.chromium.org/937643006/diff/20001/ppapi/examples/video_enc... File ppapi/examples/video_encode/video_encode.html (right): https://codereview.chromium.org/937643006/diff/20001/ppapi/examples/video_enc... ppapi/examples/video_encode/video_encode.html:4: Copyright 2014 The Chromium Authors. All rights reserved. On 2015/02/19 21:54:38, bbudge wrote: > nit: 2015 Done. https://codereview.chromium.org/937643006/diff/20001/ppapi/examples/video_enc... ppapi/examples/video_encode/video_encode.html:40: navigator.webkitGetUserMedia({audio: false, video: true}, success, failure); On 2015/02/19 21:54:38, bbudge wrote: > nit: line length > 80 Done.
On 2015/02/27 14:19:44, llandwerlin wrote: > The example is not displaying the size of the bitstream buffer, giving some > feedback on whether the thing is working. s/not/now/
LGTM w/comments https://codereview.chromium.org/937643006/diff/40001/ppapi/examples/video_enc... File ppapi/examples/video_encode/video_encode.cc (right): https://codereview.chromium.org/937643006/diff/40001/ppapi/examples/video_enc... ppapi/examples/video_encode/video_encode.cc:86: class MediaStreamVideoEncoderModule : public pp::Module { VideoEncoderModule/Instance are better names, since video encoding isn't really part of MediaStream. https://codereview.chromium.org/937643006/diff/40001/ppapi/examples/video_enc... ppapi/examples/video_encode/video_encode.cc:115: pp::VideoFrame src); nit: Either change the name to CopyFrame/CopyVideoFrame or change the parameters to encoder_frame, track_frame. https://codereview.chromium.org/937643006/diff/40001/ppapi/examples/video_enc... ppapi/examples/video_encode/video_encode.cc:342: pp::VideoFrame src) { pp::VideoFrame*, const pp::VideoFrame& ? https://codereview.chromium.org/937643006/diff/40001/ppapi/examples/video_enc... ppapi/examples/video_encode/video_encode.cc:415: LogWarning("Got track frame to encode"); s/LogWarning/Log
Thanks! https://codereview.chromium.org/937643006/diff/40001/ppapi/examples/video_enc... File ppapi/examples/video_encode/video_encode.cc (right): https://codereview.chromium.org/937643006/diff/40001/ppapi/examples/video_enc... ppapi/examples/video_encode/video_encode.cc:86: class MediaStreamVideoEncoderModule : public pp::Module { On 2015/03/02 17:52:03, bbudge wrote: > VideoEncoderModule/Instance are better names, since video encoding isn't really > part of MediaStream. Done. https://codereview.chromium.org/937643006/diff/40001/ppapi/examples/video_enc... ppapi/examples/video_encode/video_encode.cc:115: pp::VideoFrame src); On 2015/03/02 17:52:04, bbudge wrote: > nit: Either change the name to CopyFrame/CopyVideoFrame or change the parameters > to encoder_frame, track_frame. Done. https://codereview.chromium.org/937643006/diff/40001/ppapi/examples/video_enc... ppapi/examples/video_encode/video_encode.cc:342: pp::VideoFrame src) { On 2015/03/02 17:52:03, bbudge wrote: > pp::VideoFrame*, const pp::VideoFrame& ? That's not possible with GetDataBuffer() because the method is not const. https://codereview.chromium.org/937643006/diff/40001/ppapi/examples/video_enc... ppapi/examples/video_encode/video_encode.cc:415: LogWarning("Got track frame to encode"); On 2015/03/02 17:52:03, bbudge wrote: > s/LogWarning/Log Oh sorry, I should have removed this.
The CQ bit was checked by lionel.g.landwerlin@intel.com
The patchset sent to the CQ was uploaded after l-g-t-m from bbudge@chromium.org Link to the patchset: https://codereview.chromium.org/937643006/#ps60001 (title: "Final nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/937643006/60001
The CQ bit was unchecked by lionel.g.landwerlin@intel.com
Sorry, I completely missed the GN build files that were added last month.
The CQ bit was checked by lionel.g.landwerlin@intel.com
The patchset sent to the CQ was uploaded after l-g-t-m from bbudge@chromium.org Link to the patchset: https://codereview.chromium.org/937643006/#ps80001 (title: "Missing GN build files")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/937643006/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win8_chromium_rel on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
On 2015/03/02 19:38:06, I haz the power (commit-bot) wrote: > Try jobs failed on following builders: > win8_chromium_rel on tryserver.chromium.win (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...) Since Windows defines a macro 'PostMessage' you need to do something like this: https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/examples/vid...
The CQ bit was checked by lionel.g.landwerlin@intel.com
The patchset sent to the CQ was uploaded after l-g-t-m from bbudge@chromium.org Link to the patchset: https://codereview.chromium.org/937643006/#ps100001 (title: "Fix native compilation on Windows")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/937643006/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/5f3145d574f2631845867f37a38adec12dab1a7f Cr-Commit-Position: refs/heads/master@{#318785}
Message was sent while issue was closed.
On 2015/02/18 16:13:31, llandwerlin wrote: > Maybe with can get the review going on the example in parallel. Depends on > https://codereview.chromium.org/905023005/ When I run the example, it produces video that is slightly sped up. My guess is that it is capturing less than the needed 30fps (the example seems to have 30fps hard coded in places). Unfortunately when i pass in the mediaconstraints {audio: false, video:{ mandatory: { minFrameRate: 30 } } i get a ConstraintNotSatisfiedError which is strange since i am on 2015 macbook. Is this a problem with the example ? Are you able to capture 30fps ? thanks for the example, Steve
Message was sent while issue was closed.
On 2015/06/11 19:43:39, Stevefar wrote: > On 2015/02/18 16:13:31, llandwerlin wrote: > > Maybe with can get the review going on the example in parallel. Depends on > > https://codereview.chromium.org/905023005/ > > When I run the example, it produces video that is slightly sped up. My guess is > that it is capturing less than the needed 30fps (the example seems to have 30fps > hard coded in places). Unfortunately when i pass in the mediaconstraints {audio: > false, video:{ mandatory: { minFrameRate: 30 } } i get a > ConstraintNotSatisfiedError which is strange since i am on 2015 macbook. Is this > a problem with the example ? Are you able to capture 30fps ? > > thanks for the example, > Steve Hi Steve, It depends on the codec you're encoding with and also the player used to play the video. When encoding in h264, the generated file is just the concatenation of the bitstream packets returned by the encoder. There are no timestamps included in that stream. So it might be replayed faster depending on the player you're using (VLC seems to play at 30fps by default so it should be displayed correctly). A VP8 stream cannot be read without extra metadata information, so the example implements a simple IVF container which adds timestamps to the frames. So VP8 encoded videos should play at the correct speed. You will need to implement the container of your choice for the h264 bitstream (RTP or MP4 for example), it obviously depends on your application's needs. Thanks, Lionel
Message was sent while issue was closed.
On 2015/06/16 09:28:56, llandwerlin wrote: > On 2015/06/11 19:43:39, Stevefar wrote: > > On 2015/02/18 16:13:31, llandwerlin wrote: > > > Maybe with can get the review going on the example in parallel. Depends on > > > https://codereview.chromium.org/905023005/ > > > > When I run the example, it produces video that is slightly sped up. My guess > is > > that it is capturing less than the needed 30fps (the example seems to have > 30fps > > hard coded in places). Unfortunately when i pass in the mediaconstraints > {audio: > > false, video:{ mandatory: { minFrameRate: 30 } } i get a > > ConstraintNotSatisfiedError which is strange since i am on 2015 macbook. Is > this > > a problem with the example ? Are you able to capture 30fps ? > > > > thanks for the example, > > Steve > > Hi Steve, > > It depends on the codec you're encoding with and also the player used to play > the video. > > When encoding in h264, the generated file is just the concatenation of the > bitstream packets returned by the encoder. There are no timestamps included in > that stream. So it might be replayed faster depending on the player you're using > (VLC seems to play at 30fps by default so it should be displayed correctly). > > A VP8 stream cannot be read without extra metadata information, so the example > implements a simple IVF container which adds timestamps to the frames. So VP8 > encoded videos should play at the correct speed. > > You will need to implement the container of your choice for the h264 bitstream > (RTP or MP4 for example), it obviously depends on your application's needs. > > Thanks, > > Lionel Thanks Lionel, After recording a 30 second example with the vp8 codec on my macbook, downloading it, and opening it with the VLC player, the resulting video only plays 25 seconds. When I try to convert it with ffmpeg, "ffmpeg -i src_file result.mp4" the resulting video is also 25 seconds (i tried varies parameters for vsync and such). It could be that I am not understanding something. Any idea whats going on here? Thanks again, Steve
Message was sent while issue was closed.
On 2015/06/16 21:50:21, Stevefar wrote: > On 2015/06/16 09:28:56, llandwerlin wrote: > > On 2015/06/11 19:43:39, Stevefar wrote: > > > On 2015/02/18 16:13:31, llandwerlin wrote: > > > > Maybe with can get the review going on the example in parallel. Depends on > > > > https://codereview.chromium.org/905023005/ > > > > > > When I run the example, it produces video that is slightly sped up. My guess > > is > > > that it is capturing less than the needed 30fps (the example seems to have > > 30fps > > > hard coded in places). Unfortunately when i pass in the mediaconstraints > > {audio: > > > false, video:{ mandatory: { minFrameRate: 30 } } i get a > > > ConstraintNotSatisfiedError which is strange since i am on 2015 macbook. Is > > this > > > a problem with the example ? Are you able to capture 30fps ? > > > > > > thanks for the example, > > > Steve > > > > Hi Steve, > > > > It depends on the codec you're encoding with and also the player used to play > > the video. > > > > When encoding in h264, the generated file is just the concatenation of the > > bitstream packets returned by the encoder. There are no timestamps included in > > that stream. So it might be replayed faster depending on the player you're > using > > (VLC seems to play at 30fps by default so it should be displayed correctly). > > > > A VP8 stream cannot be read without extra metadata information, so the example > > implements a simple IVF container which adds timestamps to the frames. So VP8 > > encoded videos should play at the correct speed. > > > > You will need to implement the container of your choice for the h264 bitstream > > (RTP or MP4 for example), it obviously depends on your application's needs. > > > > Thanks, > > > > Lionel > > Thanks Lionel, > > After recording a 30 second example with the vp8 codec on my macbook, > downloading it, and opening it with the VLC player, the resulting video only > plays 25 seconds. When I try to convert it with ffmpeg, "ffmpeg -i src_file > result.mp4" the resulting video is also 25 seconds (i tried varies parameters > for vsync and such). It could be that I am not understanding something. Any idea > whats going on here? > > Thanks again, > Steve Thanks for bringing this to our attention Steve. The drift is not as important on my Chromebook (< 1s for a 30s stream), but here is patch to improve the accuracy of the encoding ticks in the example : https://codereview.chromium.org/1187193006 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
