Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
374153003:
Use PPS/SPS NALUs to initialize a VTDecompressionSession. (Closed)
|
Created:
6 years, 5 months ago by sandersd (OOO until July 31) Modified:
6 years, 5 months ago CC:
chromium-reviews, posciak+watch_chromium.org, jam, mcasas+watch_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org, piman+watch_chromium.org, wjia+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionUse PPS/SPS NALUs to initialize a VTDecompressionSession.
BUG=133828
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=282549
Patch Set 1 #
Total comments: 8
Patch Set 2 : Move parsing to decoder thread. #
Total comments: 19
Patch Set 3 : Use scoped pointers. #
Total comments: 7
Patch Set 4 : Fix posting to decoder thread. #
Total comments: 10
Patch Set 5 : Nits. #Patch Set 6 : gfx::Size #
Total comments: 2
Patch Set 7 : Constant for NALU header length. #
Total comments: 3
Patch Set 8 : Fix comment typo. #
Messages
Total messages: 31 (0 generated)
Quick run-by. https://codereview.chromium.org/374153003/diff/1/content/common/gpu/media/vt_... File content/common/gpu/media/vt_video_decode_accelerator.cc (right): https://codereview.chromium.org/374153003/diff/1/content/common/gpu/media/vt_... content/common/gpu/media/vt_video_decode_accelerator.cc:172: const uint8_t* buf = static_cast<uint8_t*>(memory.memory()); Please try to do as little as possible in Decode(), as this is executed on GPUProcess ChildThread an blocks other GPU tasks. It'd be better to have a separate thread for this. https://codereview.chromium.org/374153003/diff/1/content/common/gpu/media/vt_... content/common/gpu/media/vt_video_decode_accelerator.cc:176: std::vector<media::H264NALU> slice_nalus; Is this to store the slice nalus that come before SPS and PPS to use them with an SPS that comes after them? This would not be a correct stream. An SPS that comes after a slice cannot be applied to slices before it. https://codereview.chromium.org/374153003/diff/1/content/common/gpu/media/vt_... content/common/gpu/media/vt_video_decode_accelerator.cc:185: CHECK(result == media::H264Parser::kOk); It's better not to crash the GPU process on a invalid stream. Please client->NotifyError instead. https://codereview.chromium.org/374153003/diff/1/content/common/gpu/media/vt_... content/common/gpu/media/vt_video_decode_accelerator.cc:186: if (nalu.nal_unit_type >= 1 && nalu.nal_unit_type <= 5) { Please use nalu type defines instead of hardcoded numbers.
https://codereview.chromium.org/374153003/diff/1/content/common/gpu/media/vt_... File content/common/gpu/media/vt_video_decode_accelerator.cc (right): https://codereview.chromium.org/374153003/diff/1/content/common/gpu/media/vt_... content/common/gpu/media/vt_video_decode_accelerator.cc:172: const uint8_t* buf = static_cast<uint8_t*>(memory.memory()); On 2014/07/09 00:56:05, Pawel Osciak wrote: > Please try to do as little as possible in Decode(), as this is executed on > GPUProcess ChildThread an blocks other GPU tasks. It'd be better to have a > separate thread for this. Done. I've moved this into a decoder thread, similar to how the VAAPI implementation is done. https://codereview.chromium.org/374153003/diff/1/content/common/gpu/media/vt_... content/common/gpu/media/vt_video_decode_accelerator.cc:176: std::vector<media::H264NALU> slice_nalus; On 2014/07/09 00:56:05, Pawel Osciak wrote: > Is this to store the slice nalus that come before SPS and PPS to use them with > an SPS that comes after them? This would not be a correct stream. An SPS that > comes after a slice cannot be applied to slices before it. No, it's an implementation to take any that are at the start without having to parse twice, and without all the error checking done yet. https://codereview.chromium.org/374153003/diff/1/content/common/gpu/media/vt_... content/common/gpu/media/vt_video_decode_accelerator.cc:185: CHECK(result == media::H264Parser::kOk); On 2014/07/09 00:56:05, Pawel Osciak wrote: > It's better not to crash the GPU process on a invalid stream. Please > client->NotifyError instead. Indeed, but I was hoping to keep the boilerplate low for the next few CLs and then replace all the CHECKs in one go. https://codereview.chromium.org/374153003/diff/1/content/common/gpu/media/vt_... content/common/gpu/media/vt_video_decode_accelerator.cc:186: if (nalu.nal_unit_type >= 1 && nalu.nal_unit_type <= 5) { On 2014/07/09 00:56:05, Pawel Osciak wrote: > Please use nalu type defines instead of hardcoded numbers. Done. Changed to accept all types, as that is closer to the correct implementation once CanAcceptFormatDescription is used.
I'm curious, why this direction of gradually committing non-working stubs and temporary solutions? This seems to increase the burden for reviewers who need to review multiple times and can't reason about the whole picture, and I'm not sure if this is the best solution to have non-working code in Chrome, also from security perspective...
On Jul 8, 2014 7:22 PM, <posciak@chromium.org> wrote: > > I'm curious, why this direction of gradually committing non-working stubs and > temporary solutions? My goal is to get feedback on the basic design, in chunks that a reviewer can reasonably digest quickly. If the whole feature is developed and reviewed in one piece, it's harder for a reviewer to request large changes. > This seems to increase the burden for reviewers who need to review multiple > times and can't reason about the whole picture, I'm available to answer all questions, and I typically try to turn my answers into code comments, to help future developers not have to read all the code either. > and I'm not sure if this is the > best solution to have non-working code in Chrome, also from security > perspective... Good point. In this case the code is inert unless --no-sandbox is passed, in which case I expect the user to understand the risk. - Dan To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
drive by stuff posciak: I recommended sandersd@ to send out more tiny incremental CLs as he gets more familiar with Chromium. It's fairly common to develop features behind flags, so as long as both sandersd@ and reviewers are OK reviewing incremental CLs then we can proceed I believe sandersd@ does have a more fully-functional prototype. Perhaps he can upload the whole thing as a separate prototye/WIP CL so it's easier to review more incremental CLs. https://codereview.chromium.org/374153003/diff/20001/content/common/gpu/media... File content/common/gpu/media/vt_video_decode_accelerator.cc (right): https://codereview.chromium.org/374153003/diff/20001/content/common/gpu/media... content/common/gpu/media/vt_video_decode_accelerator.cc:53: // TODO(sandersd): Move these into scoped pointers. can we use base/mac/scoped_cftyperef.h now? if so might as well get the basics right https://codereview.chromium.org/374153003/diff/20001/content/common/gpu/media... content/common/gpu/media/vt_video_decode_accelerator.cc:97: if (format_ != NULL) { nit: (!format_) https://codereview.chromium.org/374153003/diff/20001/content/common/gpu/media... content/common/gpu/media/vt_video_decode_accelerator.cc:116: // TODO(sandersd): Scoped pointers. I'd go for scoped pointers right away https://codereview.chromium.org/374153003/diff/20001/content/common/gpu/media... content/common/gpu/media/vt_video_decode_accelerator.cc:142: image_config, kCVPixelBufferPixelFormatTypeKey, cf_pixel_format); AFAIK (and you may want to verify this) you can ditch CFINT(i) and the local variables by inlining the call to CFNumberCreate e.g., CFDictionarySetValue( image_config, kCVPixelBufferPixelFormatTypeKey, CFNumberCreate(kCFAllocatorDefault, kCFNumberSInt32Type, &pixel_format)); https://codereview.chromium.org/374153003/diff/20001/content/common/gpu/media... content/common/gpu/media/vt_video_decode_accelerator.cc:151: if (session_ != NULL) { !session_ https://codereview.chromium.org/374153003/diff/20001/content/common/gpu/media... content/common/gpu/media/vt_video_decode_accelerator.cc:181: // Map the bitstream buffer. DCHECK(decode_thread_.message_loop_proxy()->BelongsToCurrentThread()); https://codereview.chromium.org/374153003/diff/20001/content/common/gpu/media... content/common/gpu/media/vt_video_decode_accelerator.cc:198: CHECK(result == media::H264Parser::kOk); CHECK_EQ https://codereview.chromium.org/374153003/diff/20001/content/common/gpu/media... File content/common/gpu/media/vt_video_decode_accelerator.h (right): https://codereview.chromium.org/374153003/diff/20001/content/common/gpu/media... content/common/gpu/media/vt_video_decode_accelerator.h:19: class SingleThreadTaskRunner; de-indent (we don't indent inside namespaces) https://codereview.chromium.org/374153003/diff/20001/content/common/gpu/media... content/common/gpu/media/vt_video_decode_accelerator.h:66: scoped_refptr<base::SingleThreadTaskRunner> decoder_task_runner_; there's no need to maintain an extra task runner ref -- you can access decoder_thread_.message_loop_proxy() inside of Decode()
https://codereview.chromium.org/374153003/diff/20001/content/common/gpu/media... File content/common/gpu/media/vt_video_decode_accelerator.cc (right): https://codereview.chromium.org/374153003/diff/20001/content/common/gpu/media... content/common/gpu/media/vt_video_decode_accelerator.cc:53: // TODO(sandersd): Move these into scoped pointers. On 2014/07/09 18:32:48, scherkus wrote: > can we use base/mac/scoped_cftyperef.h now? if so might as well get the basics > right Done. https://codereview.chromium.org/374153003/diff/20001/content/common/gpu/media... content/common/gpu/media/vt_video_decode_accelerator.cc:97: if (format_ != NULL) { On 2014/07/09 18:32:48, scherkus wrote: > nit: (!format_) Done. https://codereview.chromium.org/374153003/diff/20001/content/common/gpu/media... content/common/gpu/media/vt_video_decode_accelerator.cc:116: // TODO(sandersd): Scoped pointers. On 2014/07/09 18:32:49, scherkus wrote: > I'd go for scoped pointers right away Done. https://codereview.chromium.org/374153003/diff/20001/content/common/gpu/media... content/common/gpu/media/vt_video_decode_accelerator.cc:142: image_config, kCVPixelBufferPixelFormatTypeKey, cf_pixel_format); On 2014/07/09 18:32:48, scherkus wrote: > AFAIK (and you may want to verify this) you can ditch CFINT(i) and the local > variables by inlining the call to CFNumberCreate > > e.g., > > CFDictionarySetValue( > image_config, kCVPixelBufferPixelFormatTypeKey, > CFNumberCreate(kCFAllocatorDefault, kCFNumberSInt32Type, &pixel_format)); Sadly that would leak memory, but I've gone ahead and done the conversion to scoped pointers. https://codereview.chromium.org/374153003/diff/20001/content/common/gpu/media... content/common/gpu/media/vt_video_decode_accelerator.cc:151: if (session_ != NULL) { On 2014/07/09 18:32:49, scherkus wrote: > !session_ Done. https://codereview.chromium.org/374153003/diff/20001/content/common/gpu/media... content/common/gpu/media/vt_video_decode_accelerator.cc:181: // Map the bitstream buffer. On 2014/07/09 18:32:48, scherkus wrote: > DCHECK(decode_thread_.message_loop_proxy()->BelongsToCurrentThread()); Done. https://codereview.chromium.org/374153003/diff/20001/content/common/gpu/media... content/common/gpu/media/vt_video_decode_accelerator.cc:198: CHECK(result == media::H264Parser::kOk); On 2014/07/09 18:32:48, scherkus wrote: > CHECK_EQ Done. https://codereview.chromium.org/374153003/diff/20001/content/common/gpu/media... File content/common/gpu/media/vt_video_decode_accelerator.h (right): https://codereview.chromium.org/374153003/diff/20001/content/common/gpu/media... content/common/gpu/media/vt_video_decode_accelerator.h:19: class SingleThreadTaskRunner; On 2014/07/09 18:32:49, scherkus wrote: > de-indent (we don't indent inside namespaces) Done. https://codereview.chromium.org/374153003/diff/20001/content/common/gpu/media... content/common/gpu/media/vt_video_decode_accelerator.h:66: scoped_refptr<base::SingleThreadTaskRunner> decoder_task_runner_; On 2014/07/09 18:32:49, scherkus wrote: > there's no need to maintain an extra task runner ref -- you can access > decoder_thread_.message_loop_proxy() inside of Decode() Done. It's uglier, but doesn't cause any new wrapping.
some q's -- overall looking good enough https://codereview.chromium.org/374153003/diff/20001/content/common/gpu/media... File content/common/gpu/media/vt_video_decode_accelerator.cc (right): https://codereview.chromium.org/374153003/diff/20001/content/common/gpu/media... content/common/gpu/media/vt_video_decode_accelerator.cc:142: image_config, kCVPixelBufferPixelFormatTypeKey, cf_pixel_format); On 2014/07/09 20:07:12, sandersd wrote: > On 2014/07/09 18:32:48, scherkus wrote: > > AFAIK (and you may want to verify this) you can ditch CFINT(i) and the local > > variables by inlining the call to CFNumberCreate > > > > e.g., > > > > CFDictionarySetValue( > > image_config, kCVPixelBufferPixelFormatTypeKey, > > CFNumberCreate(kCFAllocatorDefault, kCFNumberSInt32Type, &pixel_format)); > > Sadly that would leak memory, but I've gone ahead and done the conversion to > scoped pointers. Yeah I've seen both instances in the Chromium codebase. Asked on IRC to see if anyone knows. https://codereview.chromium.org/374153003/diff/40001/content/common/gpu/media... File content/common/gpu/media/vt_video_decode_accelerator.cc (right): https://codereview.chromium.org/374153003/diff/40001/content/common/gpu/media... content/common/gpu/media/vt_video_decode_accelerator.cc:115: // kVTVideoDecoderSpecification_EnableHardwareAcceleratedVideoDecoder q: can we not reference this directly? https://codereview.chromium.org/374153003/diff/40001/content/common/gpu/media... content/common/gpu/media/vt_video_decode_accelerator.cc:156: &VTVideoDecodeAccelerator::DecodeTask, weak_this_factory_.GetWeakPtr(), wouldn't this DCHECK() when running? weak_this_factory_ would be bound to the gpu thread and should DCHECK() when ran on a different thread
https://codereview.chromium.org/374153003/diff/40001/content/common/gpu/media... File content/common/gpu/media/vt_video_decode_accelerator.cc (right): https://codereview.chromium.org/374153003/diff/40001/content/common/gpu/media... content/common/gpu/media/vt_video_decode_accelerator.cc:115: // kVTVideoDecoderSpecification_EnableHardwareAcceleratedVideoDecoder On 2014/07/09 20:37:44, scherkus wrote: > q: can we not reference this directly? I didn't find a way to have the stub generator link non-function symbols. https://codereview.chromium.org/374153003/diff/40001/content/common/gpu/media... content/common/gpu/media/vt_video_decode_accelerator.cc:156: &VTVideoDecodeAccelerator::DecodeTask, weak_this_factory_.GetWeakPtr(), On 2014/07/09 20:37:44, scherkus wrote: > wouldn't this DCHECK() when running? > > weak_this_factory_ would be bound to the gpu thread and should DCHECK() when ran > on a different thread My understanding of the WeakPtrFactory is that only dereferencing and invalidation are bound to a particular thread. The only alternative I see is saving a weak pointer in the class, which would then need to be copied by Bind, and I don't see how that would be different. The code has a few layers of indirection though, I could easily be misunderstanding.
https://codereview.chromium.org/374153003/diff/40001/content/common/gpu/media... File content/common/gpu/media/vt_video_decode_accelerator.cc (right): https://codereview.chromium.org/374153003/diff/40001/content/common/gpu/media... content/common/gpu/media/vt_video_decode_accelerator.cc:156: &VTVideoDecodeAccelerator::DecodeTask, weak_this_factory_.GetWeakPtr(), On 2014/07/09 20:52:50, sandersd wrote: > On 2014/07/09 20:37:44, scherkus wrote: > > wouldn't this DCHECK() when running? > > > > weak_this_factory_ would be bound to the gpu thread and should DCHECK() when > ran > > on a different thread > > My understanding of the WeakPtrFactory is that only dereferencing and > invalidation are bound to a particular thread. The only alternative I see is > saving a weak pointer in the class, which would then need to be copied by Bind, > and I don't see how that would be different. > > The code has a few layers of indirection though, I could easily be > misunderstanding. There's a bit of subtleness with how WeakPtr{Factory} work, namely that they get bound to the thread where the first dereference happens: https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/weak_p... The most typical usage is to hand out a WeakPtr-using-callback to a different thread that will only get run back on the *original thread* where the PostTask was made. In your case, the WeakPtrFactory's first dereference is on the |decoder_thread_|, which isn't the right thread we want our WeakPtrs to be bound to. When VTVDA gets deleted on the GPU thread, it'll invoke ~WeakPtrFactory() ... but it's been bound to the |decoder_thread_| and we'll DCHECK. Here's a code sample that hopefully illustrates the problem: class Bar { public: Bar() {} void TestFirstDereferenceAfterPosting() { base::WeakPtrFactory<Bar> weak_factory(this); base::WeakPtr<Bar> weak_this = weak_factory.GetWeakPtr(); base::Thread thread("BarThread"); CHECK(thread.Start()); base::WaitableEvent event(false, false); thread.message_loop_proxy()->PostTask( FROM_HERE, base::Bind(&Bar::Signal, weak_this, &event)); event.Wait(); // |weak_factory| has now been bound to |thread|. // Simulate ~WeakPtrFactory<T>(). This will DCHECK(). weak_factory.InvalidateWeakPtrs(); } // This will cause WeakPtrFactory to get bound to wrong thread. void Signal(base::WaitableEvent* event) { event->Signal(); } private: DISALLOW_COPY_AND_ASSIGN(Bar); }; The solution here is to use base::Unretained(this) when posting work to |decoder_thread_|. Since VTVDA owns |decoder_thread_| and stops it inside of ~VTVDA(), you can guarantee that all work posted to that thread will complete before VTVDA is destroyed and we won't use-after-free. Do note that you'll still need to use WeakPtrs when posting work from |decoder_thread_| *back* to the GPU thread ... but as far as this CL is concerned you don't need to worry about that just yet.
https://codereview.chromium.org/374153003/diff/40001/content/common/gpu/media... File content/common/gpu/media/vt_video_decode_accelerator.cc (right): https://codereview.chromium.org/374153003/diff/40001/content/common/gpu/media... content/common/gpu/media/vt_video_decode_accelerator.cc:156: &VTVideoDecodeAccelerator::DecodeTask, weak_this_factory_.GetWeakPtr(), > In your case, the WeakPtrFactory's first dereference is on the > |decoder_thread_|, which isn't the right thread we want our WeakPtrs to be bound > to. Done. Blindingly obvious now.
lgtm w/ nits I'd like to start thinking of unit/integration tests sooner rather than later to get some basic form of code coverage. Even getting a test up that verifies none of the code crashes would be a good start. https://codereview.chromium.org/374153003/diff/40001/content/common/gpu/media... File content/common/gpu/media/vt_video_decode_accelerator.cc (right): https://codereview.chromium.org/374153003/diff/40001/content/common/gpu/media... content/common/gpu/media/vt_video_decode_accelerator.cc:156: &VTVideoDecodeAccelerator::DecodeTask, weak_this_factory_.GetWeakPtr(), On 2014/07/09 22:27:37, sandersd wrote: > > In your case, the WeakPtrFactory's first dereference is on the > > |decoder_thread_|, which isn't the right thread we want our WeakPtrs to be > bound > > to. > > Done. > > Blindingly obvious now. haha yeah it's all a matter of learning the in's-and-out's of Chromium's threading/callback primitives ... it can take some time :) https://codereview.chromium.org/374153003/diff/60001/content/common/gpu/media... File content/common/gpu/media/vt_video_decode_accelerator.cc (right): https://codereview.chromium.org/374153003/diff/60001/content/common/gpu/media... content/common/gpu/media/vt_video_decode_accelerator.cc:128: #define CFINT(i) CFNumberCreate(kCFAllocatorDefault, kCFNumberSInt32Type, &i) nit: it's good practice to #undef when you're done using a helper macro https://codereview.chromium.org/374153003/diff/60001/content/common/gpu/media... File content/common/gpu/media/vt_video_decode_accelerator.h (right): https://codereview.chromium.org/374153003/diff/60001/content/common/gpu/media... content/common/gpu/media/vt_video_decode_accelerator.h:63: scoped_refptr<base::SingleThreadTaskRunner> gpu_task_runner_; nit: I'd just get rid of this until you need it
https://codereview.chromium.org/374153003/diff/60001/content/common/gpu/media... File content/common/gpu/media/vt_video_decode_accelerator.cc (right): https://codereview.chromium.org/374153003/diff/60001/content/common/gpu/media... content/common/gpu/media/vt_video_decode_accelerator.cc:35: vda->Output(bitstream_id, status, info_flags, image_buffer); What guarantees that vda is still valid (haven't been Destroy()ed) when this is called? Do you need a weak pointer? Or does the user of this always run on decoder_thread_ ? https://codereview.chromium.org/374153003/diff/60001/content/common/gpu/media... content/common/gpu/media/vt_video_decode_accelerator.cc:172: std::vector<media::H264NALU> nalus; Unused? https://codereview.chromium.org/374153003/diff/60001/content/common/gpu/media... content/common/gpu/media/vt_video_decode_accelerator.cc:189: data_size += 4 + nalu.size; What is "4" for? Also, data_size seems unused in general... https://codereview.chromium.org/374153003/diff/60001/content/common/gpu/media... File content/common/gpu/media/vt_video_decode_accelerator.h (right): https://codereview.chromium.org/374153003/diff/60001/content/common/gpu/media... content/common/gpu/media/vt_video_decode_accelerator.h:73: int32_t coded_width_; Nit: use gfx::Size ?
> I'd like to start thinking of unit/integration tests sooner rather than later to > get some basic form of code coverage. Even getting a test up that verifies none > of the code crashes would be a good start. There is a video_decode_accelerator_unittest that tests all the basic functions, but I can't get it to compile at all. Getting that working will be a priority. I'll add a sanity-checking unittest tomorrow (to this issue). https://codereview.chromium.org/374153003/diff/60001/content/common/gpu/media... File content/common/gpu/media/vt_video_decode_accelerator.cc (right): https://codereview.chromium.org/374153003/diff/60001/content/common/gpu/media... content/common/gpu/media/vt_video_decode_accelerator.cc:35: vda->Output(bitstream_id, status, info_flags, image_buffer); On 2014/07/10 00:10:55, Pawel Osciak wrote: > What guarantees that vda is still valid (haven't been Destroy()ed) when this is > called? Do you need a weak pointer? Or does the user of this always run on > decoder_thread_ ? The VTDecompressionSession will be flushed in Destroy(), after which no callbacks will be made. https://codereview.chromium.org/374153003/diff/60001/content/common/gpu/media... content/common/gpu/media/vt_video_decode_accelerator.cc:128: #define CFINT(i) CFNumberCreate(kCFAllocatorDefault, kCFNumberSInt32Type, &i) On 2014/07/09 22:39:25, scherkus wrote: > nit: it's good practice to #undef when you're done using a helper macro Done. https://codereview.chromium.org/374153003/diff/60001/content/common/gpu/media... content/common/gpu/media/vt_video_decode_accelerator.cc:172: std::vector<media::H264NALU> nalus; On 2014/07/10 00:10:55, Pawel Osciak wrote: > Unused? Currently unused, will be used as the source of pointers to copy into a CMBlockBuffer. See https://codereview.chromium.org/339773002/diff/60001/content/common/gpu/media... for an implementation. https://codereview.chromium.org/374153003/diff/60001/content/common/gpu/media... content/common/gpu/media/vt_video_decode_accelerator.cc:189: data_size += 4 + nalu.size; On 2014/07/10 00:10:55, Pawel Osciak wrote: > What is "4" for? Also, data_size seems unused in general... Added comment to explain.
On 2014/07/10 00:35:38, sandersd wrote: > > I'd like to start thinking of unit/integration tests sooner rather than later > to > > get some basic form of code coverage. Even getting a test up that verifies > none > > of the code crashes would be a good start. > > There is a video_decode_accelerator_unittest that tests all the basic functions, > but I can't get it to compile at all. Getting that working will be a priority. > Feel free to ping me on chat if you need help with getting the test to work.
https://codereview.chromium.org/374153003/diff/100001/content/common/gpu/medi... File content/common/gpu/media/vt_video_decode_accelerator.cc (right): https://codereview.chromium.org/374153003/diff/100001/content/common/gpu/medi... content/common/gpu/media/vt_video_decode_accelerator.cc:186: // Each NALU will have a 4-byte length header prepended. Is this header something VT-specific?
https://codereview.chromium.org/374153003/diff/100001/content/common/gpu/medi... File content/common/gpu/media/vt_video_decode_accelerator.cc (right): https://codereview.chromium.org/374153003/diff/100001/content/common/gpu/medi... content/common/gpu/media/vt_video_decode_accelerator.cc:186: // Each NALU will have a 4-byte length header prepended. On 2014/07/10 00:43:12, Pawel Osciak wrote: > Is this header something VT-specific? Yes, VT expects AVCC formatting.
On 2014/07/10 00:47:52, sandersd wrote: > https://codereview.chromium.org/374153003/diff/100001/content/common/gpu/medi... > File content/common/gpu/media/vt_video_decode_accelerator.cc (right): > > https://codereview.chromium.org/374153003/diff/100001/content/common/gpu/medi... > content/common/gpu/media/vt_video_decode_accelerator.cc:186: // Each NALU will > have a 4-byte length header prepended. > On 2014/07/10 00:43:12, Pawel Osciak wrote: > > Is this header something VT-specific? > > Yes, VT expects AVCC formatting. I see. Would be good to make it a constant then and point to the spec please.
> > > Is this header something VT-specific? > > > > Yes, VT expects AVCC formatting. > > I see. Would be good to make it a constant then and point to the spec please. I don't have any access to the MPEG-4 spec, and VT itself is undocumented, but I have gone ahead and added a constant for this.
https://codereview.chromium.org/374153003/diff/50004/content/common/gpu/media... File content/common/gpu/media/vt_video_decode_accelerator.cc (right): https://codereview.chromium.org/374153003/diff/50004/content/common/gpu/media... content/common/gpu/media/vt_video_decode_accelerator.cc:21: static const int kNALUHeaderLength = 4; Sorry, I'm still interested in more details. Is this Annex-B header with NALU type and ref_idc? Or do you mean NALU start code? Why do you need to rewrite it instead of taking what is in the stream?
On 2014/07/10 01:14:13, Pawel Osciak wrote: > https://codereview.chromium.org/374153003/diff/50004/content/common/gpu/media... > File content/common/gpu/media/vt_video_decode_accelerator.cc (right): > > https://codereview.chromium.org/374153003/diff/50004/content/common/gpu/media... > content/common/gpu/media/vt_video_decode_accelerator.cc:21: static const int > kNALUHeaderLength = 4; > Sorry, I'm still interested in more details. Is this Annex-B header with NALU > type and ref_idc? Or do you mean NALU start code? > Why do you need to rewrite it instead of taking what is in the stream? The best explanation I found was this one, which covers both kinds of framing in two sections: http://www.szatmary.org/blog/25 The data comes in with AnnexB framing, and VT expects AVCC aka MPEG-4 framing. This means that start codes are replaced by size headers (the length of which is part of the avcC metadata record), but that emulation prevention bytes are left alone. (It also means that SPS and PPS NALUs should be dropped from the stream, but so far VT seems to be forgiving.) There is a bad explanation of this in the 2014 WDCC slides from "Direct Access to Video Encoding and Decoding" (I can send you a copy if you like), but no other documentation from Apple.
On 2014/07/10 01:28:47, sandersd wrote: > On 2014/07/10 01:14:13, Pawel Osciak wrote: > > > https://codereview.chromium.org/374153003/diff/50004/content/common/gpu/media... > > File content/common/gpu/media/vt_video_decode_accelerator.cc (right): > > > > > https://codereview.chromium.org/374153003/diff/50004/content/common/gpu/media... > > content/common/gpu/media/vt_video_decode_accelerator.cc:21: static const int > > kNALUHeaderLength = 4; > > Sorry, I'm still interested in more details. Is this Annex-B header with NALU > > type and ref_idc? Or do you mean NALU start code? > > Why do you need to rewrite it instead of taking what is in the stream? > > The best explanation I found was this one, which covers both kinds of framing in > two sections: http://www.szatmary.org/blog/25 > > The data comes in with AnnexB framing, and VT expects AVCC aka MPEG-4 framing. > This means that start codes are replaced by size headers (the length of which is > part of the avcC metadata record), but that emulation prevention bytes are left > alone. (It also means that SPS and PPS NALUs should be dropped from the stream, > but so far VT seems to be forgiving.) > > There is a bad explanation of this in the 2014 WDCC slides from "Direct Access > to Video Encoding and Decoding" (I can send you a copy if you like), but no > other documentation from Apple. Thanks for the detailed explanation. Makes sense! I'd add this to the comments for the constant/code that adds those (can be done later). Otherwise lgtm. Thanks for your patience.
I'd defer H264 related code to posciak. lgtm % tiny nit https://codereview.chromium.org/374153003/diff/50004/content/common/gpu/media... File content/common/gpu/media/vt_video_decode_accelerator.cc (right): https://codereview.chromium.org/374153003/diff/50004/content/common/gpu/media... content/common/gpu/media/vt_video_decode_accelerator.cc:139: // TODO(sandersd): Skip if the session is compatable. typo: compatible?
After discussing with Xiaohan, I've decided to skip the unittest for now in favor of getting video_decode_accelerator_unittest working (which appears to do exactly what we need). I've separately started a CL to implement CGL support in that test (currently it handles GLX and EGL). https://codereview.chromium.org/374153003/diff/50004/content/common/gpu/media... File content/common/gpu/media/vt_video_decode_accelerator.cc (right): https://codereview.chromium.org/374153003/diff/50004/content/common/gpu/media... content/common/gpu/media/vt_video_decode_accelerator.cc:139: // TODO(sandersd): Skip if the session is compatable. On 2014/07/10 19:04:24, xhwang wrote: > typo: compatible? Done.
The CQ bit was checked by sandersd@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sandersd@chromium.org/374153003/130001
Message was sent while issue was closed.
Change committed as 282549
Message was sent while issue was closed.
On my mac build fails due to this change with message: tools/build/mac/verify_order: unordered symbols in /Users/sergeyv/chr/src/out/Release/Chromium Framework.framework/Versions/A/Chromium Framework: _CMBlockBufferCreateWithMemoryBlock _CMBlockBufferReplaceDataBytes _CMSampleBufferCreate _CMVideoFormatDescriptionGetDimensions.
Message was sent while issue was closed.
On 2014/07/12 15:20:04, sergeyv wrote: > On my mac build fails due to this change with message: > tools/build/mac/verify_order: unordered symbols in > /Users/sergeyv/chr/src/out/Release/Chromium > Framework.framework/Versions/A/Chromium Framework: > _CMBlockBufferCreateWithMemoryBlock > _CMBlockBufferReplaceDataBytes > _CMSampleBufferCreate > _CMVideoFormatDescriptionGetDimensions. http://build.chromium.org/p/chromium.swarm/waterfall?reload=120&builder=Mac%2... has been broken since that CL. It needs to be looked into ASAP or be reverted.
Message was sent while issue was closed.
On 2014/07/14 21:11:09, M-A Ruel wrote: > On 2014/07/12 15:20:04, sergeyv wrote: > > On my mac build fails due to this change with message: > > tools/build/mac/verify_order: unordered symbols in > > /Users/sergeyv/chr/src/out/Release/Chromium > > Framework.framework/Versions/A/Chromium Framework: > > _CMBlockBufferCreateWithMemoryBlock > > _CMBlockBufferReplaceDataBytes > > _CMSampleBufferCreate > > _CMVideoFormatDescriptionGetDimensions. > > http://build.chromium.org/p/chromium.swarm/waterfall?reload=120&builder=Mac%2... > has been broken since that CL. It needs to be looked into ASAP or be reverted. Fix is in https://codereview.chromium.org/393633002/, but it's taking a while to get through the bots. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
