Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
691233002:
Added aac bitstream converter (Closed)
DescriptionAdded aac bitstream converter
Standardizes aac stream to have adts header for every segment
Also:
- Abstracted ffmpeg bitstream class
- Moved converter creation into EnableBitstreamConverter
BUG=431167
Committed: https://crrev.com/c04b6087078aa371c0407ed3c2b93b6375718f42
Cr-Commit-Position: refs/heads/master@{#303325}
Patch Set 1 #Patch Set 2 : #
Total comments: 26
Patch Set 3 : #
Total comments: 12
Patch Set 4 : #Patch Set 5 : #
Total comments: 12
Patch Set 6 : Added aac converter unittest #
Total comments: 14
Patch Set 7 : Final nits #Patch Set 8 : Single comment change #Patch Set 9 : Build error fixes #Patch Set 10 : Once more with feeling... #Patch Set 11 : #Patch Set 12 : Figured it out #
Total comments: 2
Messages
Total messages: 42 (12 generated)
kjoswiak@chromium.org changed reviewers: + dalecurtis@chromium.org, xhwang@chromium.org
Alright main thing here is an upstream of aac bitstream converter used for chromecast. I also included a change to move creation of bitstream to be on enable, but can either revert that part or split into a separate CL if preferred
Do we really want to do this for non-Chromecast clients? Why is Chromecast doing this anyways? AAC seemingly works fine without this...
On 2014/10/30 23:04:23, DaleCurtis wrote: > Do we really want to do this for non-Chromecast clients? Why is Chromecast > doing this anyways? AAC seemingly works fine without this... Its only used if enable called on AAC stream, which should only happen for chromecast (in code that has not been upstreamed). It needs adts headers for hardware decoder
https://codereview.chromium.org/691233002/diff/20001/media/filters/ffmpeg_aac... File media/filters/ffmpeg_aac_bitstream_converter.cc (right): https://codereview.chromium.org/691233002/diff/20001/media/filters/ffmpeg_aac... media/filters/ffmpeg_aac_bitstream_converter.cc:27: LOG(ERROR) << "extradata is null"; DLOG(ERROR) https://codereview.chromium.org/691233002/diff/20001/media/filters/ffmpeg_aac... media/filters/ffmpeg_aac_bitstream_converter.cc:31: LOG(ERROR) << "extradata too small to contain MP4A header"; Ditto. https://codereview.chromium.org/691233002/diff/20001/media/filters/ffmpeg_aac... media/filters/ffmpeg_aac_bitstream_converter.cc:40: header_generated_ = GenerateAdtsHeader(stream_context_->codec_id, Seems most of this is just 0s, so why even provide them to the method, just always use zero. https://codereview.chromium.org/691233002/diff/20001/media/filters/ffmpeg_aac... media/filters/ffmpeg_aac_bitstream_converter.cc:64: memcpy(dest_packet.data, hdr_, kAdtsHeaderSize * sizeof(unsigned char)); Why copy the header versus generating it directly into dest_packet.data? https://codereview.chromium.org/691233002/diff/20001/media/filters/ffmpeg_aac... media/filters/ffmpeg_aac_bitstream_converter.cc:71: dest_packet.pts = packet->pts; Can you use av_packet_copy_props() instead? Thats what we do for H264 now. https://codereview.chromium.org/691233002/diff/20001/media/filters/ffmpeg_aac... media/filters/ffmpeg_aac_bitstream_converter.cc:115: // Syncword Instead of writing raw bytes, why not create have an adts struct with proper member variables? https://codereview.chromium.org/691233002/diff/20001/media/filters/ffmpeg_aac... media/filters/ffmpeg_aac_bitstream_converter.cc:121: case CODEC_ID_AAC: Seems something you could DCHECK() instead. https://codereview.chromium.org/691233002/diff/20001/media/filters/ffmpeg_aac... media/filters/ffmpeg_aac_bitstream_converter.cc:189: LOG(ERROR) << "[" << __FUNCTION__ << "] " DLOG https://codereview.chromium.org/691233002/diff/20001/media/filters/ffmpeg_aac... File media/filters/ffmpeg_aac_bitstream_converter.h (right): https://codereview.chromium.org/691233002/diff/20001/media/filters/ffmpeg_aac... media/filters/ffmpeg_aac_bitstream_converter.h:21: Remove line. No need to repeat the class name in the class comment :) https://codereview.chromium.org/691233002/diff/20001/media/filters/ffmpeg_aac... media/filters/ffmpeg_aac_bitstream_converter.h:35: AVCodecContext* stream_context_; It's a codec context not an AVStream context, which makes this confusingly named. https://codereview.chromium.org/691233002/diff/20001/media/filters/ffmpeg_aac... media/filters/ffmpeg_aac_bitstream_converter.h:37: static int const kAdtsHeaderSize = 7; enum, no statics in header file, every include gets its own copy. https://codereview.chromium.org/691233002/diff/20001/media/filters/ffmpeg_aac... media/filters/ffmpeg_aac_bitstream_converter.h:39: unsigned char hdr_[kAdtsHeaderSize]; uint8_t instead.
Also is ffmpeg_bitstream_converter_.h in the right place? Its the interface for bitstreams, not sure if it should be declared in an existing ffmpeg header or if the header belongs elsewhere. https://codereview.chromium.org/691233002/diff/20001/media/filters/ffmpeg_aac... File media/filters/ffmpeg_aac_bitstream_converter.cc (right): https://codereview.chromium.org/691233002/diff/20001/media/filters/ffmpeg_aac... media/filters/ffmpeg_aac_bitstream_converter.cc:27: LOG(ERROR) << "extradata is null"; On 2014/10/31 01:03:33, DaleCurtis wrote: > DLOG(ERROR) Done. https://codereview.chromium.org/691233002/diff/20001/media/filters/ffmpeg_aac... media/filters/ffmpeg_aac_bitstream_converter.cc:31: LOG(ERROR) << "extradata too small to contain MP4A header"; On 2014/10/31 01:03:33, DaleCurtis wrote: > Ditto. Done. https://codereview.chromium.org/691233002/diff/20001/media/filters/ffmpeg_aac... media/filters/ffmpeg_aac_bitstream_converter.cc:40: header_generated_ = GenerateAdtsHeader(stream_context_->codec_id, On 2014/10/31 01:03:33, DaleCurtis wrote: > Seems most of this is just 0s, so why even provide them to the method, just > always use zero. I think reasoning (this is not my code originally, I'm just upstreaming) is its easier to change if some of those zero fields become relevant, and more verbosely reveals adts spec, and I don't think we should necessarily remove that. https://codereview.chromium.org/691233002/diff/20001/media/filters/ffmpeg_aac... media/filters/ffmpeg_aac_bitstream_converter.cc:64: memcpy(dest_packet.data, hdr_, kAdtsHeaderSize * sizeof(unsigned char)); On 2014/10/31 01:03:33, DaleCurtis wrote: > Why copy the header versus generating it directly into dest_packet.data? I believe thinking is we generate header once, and then keep it around. So we generate to class member first run only, and then just copy over on future calls. https://codereview.chromium.org/691233002/diff/20001/media/filters/ffmpeg_aac... media/filters/ffmpeg_aac_bitstream_converter.cc:71: dest_packet.pts = packet->pts; On 2014/10/31 01:03:33, DaleCurtis wrote: > Can you use av_packet_copy_props() instead? Thats what we do for H264 now. Done. https://codereview.chromium.org/691233002/diff/20001/media/filters/ffmpeg_aac... media/filters/ffmpeg_aac_bitstream_converter.cc:115: // Syncword On 2014/10/31 01:03:33, DaleCurtis wrote: > Instead of writing raw bytes, why not create have an adts struct with proper > member variables? Conceptually hdr_ is raw bytes, its the bytes that get directly memcpyed into packet. A struct would obfuscate this fact (and likely would not guarantee right mem layout) https://codereview.chromium.org/691233002/diff/20001/media/filters/ffmpeg_aac... media/filters/ffmpeg_aac_bitstream_converter.cc:121: case CODEC_ID_AAC: On 2014/10/31 01:03:33, DaleCurtis wrote: > Seems something you could DCHECK() instead. Done. https://codereview.chromium.org/691233002/diff/20001/media/filters/ffmpeg_aac... media/filters/ffmpeg_aac_bitstream_converter.cc:189: LOG(ERROR) << "[" << __FUNCTION__ << "] " On 2014/10/31 01:03:33, DaleCurtis wrote: > DLOG Done. https://codereview.chromium.org/691233002/diff/20001/media/filters/ffmpeg_aac... File media/filters/ffmpeg_aac_bitstream_converter.h (right): https://codereview.chromium.org/691233002/diff/20001/media/filters/ffmpeg_aac... media/filters/ffmpeg_aac_bitstream_converter.h:21: On 2014/10/31 01:03:33, DaleCurtis wrote: > Remove line. No need to repeat the class name in the class comment :) Done. https://codereview.chromium.org/691233002/diff/20001/media/filters/ffmpeg_aac... media/filters/ffmpeg_aac_bitstream_converter.h:35: AVCodecContext* stream_context_; On 2014/10/31 01:03:33, DaleCurtis wrote: > It's a codec context not an AVStream context, which makes this confusingly > named. Its used this way in the 264 one as well. For now changing both to use stream_codec_context_, captures both ownership to stream and better mirrors class name. Could also reduce to context_ for conciseness. Let me know your preference https://codereview.chromium.org/691233002/diff/20001/media/filters/ffmpeg_aac... media/filters/ffmpeg_aac_bitstream_converter.h:37: static int const kAdtsHeaderSize = 7; On 2014/10/31 01:03:33, DaleCurtis wrote: > enum, no statics in header file, every include gets its own copy. Done. https://codereview.chromium.org/691233002/diff/20001/media/filters/ffmpeg_aac... media/filters/ffmpeg_aac_bitstream_converter.h:39: unsigned char hdr_[kAdtsHeaderSize]; On 2014/10/31 01:03:33, DaleCurtis wrote: > uint8_t instead. Done.
https://codereview.chromium.org/691233002/diff/20001/media/filters/ffmpeg_aac... File media/filters/ffmpeg_aac_bitstream_converter.cc (right): https://codereview.chromium.org/691233002/diff/20001/media/filters/ffmpeg_aac... media/filters/ffmpeg_aac_bitstream_converter.cc:64: memcpy(dest_packet.data, hdr_, kAdtsHeaderSize * sizeof(unsigned char)); On 2014/10/31 18:37:22, kjoswiak wrote: > On 2014/10/31 01:03:33, DaleCurtis wrote: > > Why copy the header versus generating it directly into dest_packet.data? > > I believe thinking is we generate header once, and then keep it around. So we > generate to class member first run only, and then just copy over on future > calls. Ah right, my mistake. I thought it was being generated every time. https://codereview.chromium.org/691233002/diff/20001/media/filters/ffmpeg_aac... media/filters/ffmpeg_aac_bitstream_converter.cc:115: // Syncword On 2014/10/31 18:37:22, kjoswiak wrote: > On 2014/10/31 01:03:33, DaleCurtis wrote: > > Instead of writing raw bytes, why not create have an adts struct with proper > > member variables? > > Conceptually hdr_ is raw bytes, its the bytes that get directly memcpyed into > packet. A struct would obfuscate this fact (and likely would not guarantee right > mem layout) I see. You could accomplish this with unions and careful construction I think, but I agree it's not the most helpful https://codereview.chromium.org/691233002/diff/40001/media/filters/ffmpeg_aac... File media/filters/ffmpeg_aac_bitstream_converter.cc (right): https://codereview.chromium.org/691233002/diff/40001/media/filters/ffmpeg_aac... media/filters/ffmpeg_aac_bitstream_converter.cc:24: packet->size + kAdtsHeaderSize * sizeof(unsigned char); Is the * sizeof(unsigned char) necessary? kAdtsHeaderSize is in bytes no? Ditto for everywhere else. https://codereview.chromium.org/691233002/diff/40001/media/filters/ffmpeg_aac... media/filters/ffmpeg_aac_bitstream_converter.cc:26: if (stream_codec_context_->extradata == 0) { If !extradata https://codereview.chromium.org/691233002/diff/40001/media/filters/ffmpeg_aac... media/filters/ffmpeg_aac_bitstream_converter.cc:85: DCHECK(codec == CODEC_ID_AAC); DCHECK_EQ https://codereview.chromium.org/691233002/diff/40001/media/filters/ffmpeg_aac... media/filters/ffmpeg_aac_bitstream_converter.cc:87: memset(reinterpret_cast<void *>(hdr_), 0, Up to you, but you can zero initialize hdr_ by specifying it in the constructor as hdr_(), https://codereview.chromium.org/691233002/diff/40001/media/filters/ffmpeg_aac... File media/filters/ffmpeg_aac_bitstream_converter.h (right): https://codereview.chromium.org/691233002/diff/40001/media/filters/ffmpeg_aac... media/filters/ffmpeg_aac_bitstream_converter.h:17: // Adds ADTS headers to AAC frames. Can you reflow this comment to avoid the odd line breaks? https://codereview.chromium.org/691233002/diff/40001/media/filters/ffmpeg_bit... File media/filters/ffmpeg_bitstream_converter.h (right): https://codereview.chromium.org/691233002/diff/40001/media/filters/ffmpeg_bit... media/filters/ffmpeg_bitstream_converter.h:14: class FFmpegBitstreamConverter { Needs docs.
https://codereview.chromium.org/691233002/diff/40001/media/filters/ffmpeg_aac... File media/filters/ffmpeg_aac_bitstream_converter.cc (right): https://codereview.chromium.org/691233002/diff/40001/media/filters/ffmpeg_aac... media/filters/ffmpeg_aac_bitstream_converter.cc:24: packet->size + kAdtsHeaderSize * sizeof(unsigned char); On 2014/10/31 21:50:15, DaleCurtis wrote: > Is the * sizeof(unsigned char) necessary? kAdtsHeaderSize is in bytes no? > Ditto for everywhere else. Done. https://codereview.chromium.org/691233002/diff/40001/media/filters/ffmpeg_aac... media/filters/ffmpeg_aac_bitstream_converter.cc:26: if (stream_codec_context_->extradata == 0) { On 2014/10/31 21:50:15, DaleCurtis wrote: > If !extradata Done. https://codereview.chromium.org/691233002/diff/40001/media/filters/ffmpeg_aac... media/filters/ffmpeg_aac_bitstream_converter.cc:85: DCHECK(codec == CODEC_ID_AAC); On 2014/10/31 21:50:15, DaleCurtis wrote: > DCHECK_EQ Done. https://codereview.chromium.org/691233002/diff/40001/media/filters/ffmpeg_aac... media/filters/ffmpeg_aac_bitstream_converter.cc:87: memset(reinterpret_cast<void *>(hdr_), 0, On 2014/10/31 21:50:15, DaleCurtis wrote: > Up to you, but you can zero initialize hdr_ by specifying it in the constructor > as hdr_(), I kind of like it this way, ensures is zeroed precisely when it needs to be and does the work on demand https://codereview.chromium.org/691233002/diff/40001/media/filters/ffmpeg_aac... File media/filters/ffmpeg_aac_bitstream_converter.h (right): https://codereview.chromium.org/691233002/diff/40001/media/filters/ffmpeg_aac... media/filters/ffmpeg_aac_bitstream_converter.h:17: // Adds ADTS headers to AAC frames. On 2014/10/31 21:50:15, DaleCurtis wrote: > Can you reflow this comment to avoid the odd line breaks? Done. https://codereview.chromium.org/691233002/diff/40001/media/filters/ffmpeg_bit... File media/filters/ffmpeg_bitstream_converter.h (right): https://codereview.chromium.org/691233002/diff/40001/media/filters/ffmpeg_bit... media/filters/ffmpeg_bitstream_converter.h:14: class FFmpegBitstreamConverter { On 2014/10/31 21:50:15, DaleCurtis wrote: > Needs docs. Done.
lgtm % adding these files to the BUILD.gn as well.
Alright, anything else? xhwang, does this look good to you? I especially want to make sure the change in converter creation looks fine
Status? Should I wait on xhwang or is it alright to commit?
Thank you for working on this! Looking pretty good. I had some nits but nothing major. Can you add some unittests similar to ffmpeg_h264_to_annex_b_bitstream_converter_unittest.cc? Also, can you file a bug explaining why this is needed? That'll help people better understand this code. https://codereview.chromium.org/691233002/diff/80001/media/filters/ffmpeg_aac... File media/filters/ffmpeg_aac_bitstream_converter.h (right): https://codereview.chromium.org/691233002/diff/80001/media/filters/ffmpeg_aac... media/filters/ffmpeg_aac_bitstream_converter.h:25: // FFmpegBitstreamConverter implementation. https://codereview.chromium.org/691233002/diff/80001/media/filters/ffmpeg_aac... media/filters/ffmpeg_aac_bitstream_converter.h:45: int number_of_frames_minus_one); nit: You can make this a file scoped static function in the cc file. The new function can return the header, which will be more clear. https://codereview.chromium.org/691233002/diff/80001/media/filters/ffmpeg_bit... File media/filters/ffmpeg_bitstream_converter.h (right): https://codereview.chromium.org/691233002/diff/80001/media/filters/ffmpeg_bit... media/filters/ffmpeg_bitstream_converter.h:21: virtual bool ConvertPacket(AVPacket* packet) = 0; What's the return value? https://codereview.chromium.org/691233002/diff/80001/media/filters/ffmpeg_dem... File media/filters/ffmpeg_demuxer.h (right): https://codereview.chromium.org/691233002/diff/80001/media/filters/ffmpeg_dem... media/filters/ffmpeg_demuxer.h:53: class FFmpegBitstreamConverter; Fix order. https://codereview.chromium.org/691233002/diff/80001/media/filters/ffmpeg_dem... media/filters/ffmpeg_demuxer.h:130: // Create new bitstream converter, destroying active converter if present Add period after "present". https://codereview.chromium.org/691233002/diff/80001/media/filters/ffmpeg_h26... File media/filters/ffmpeg_h264_to_annex_b_bitstream_converter.h (right): https://codereview.chromium.org/691233002/diff/80001/media/filters/ffmpeg_h26... media/filters/ffmpeg_h264_to_annex_b_bitstream_converter.h:31: // Converts |packet| to H.264 Annex B bytestream format. This conversion is // FFmpegBitstreamConverter implementation.
Alright, addressed nits, attached bug, and submitted some basic unit tests for aac converter. Really not a ton to test there, its doing substantially less than the h264 converter seems to be doing https://codereview.chromium.org/691233002/diff/80001/media/filters/ffmpeg_aac... File media/filters/ffmpeg_aac_bitstream_converter.h (right): https://codereview.chromium.org/691233002/diff/80001/media/filters/ffmpeg_aac... media/filters/ffmpeg_aac_bitstream_converter.h:25: On 2014/11/06 04:35:28, xhwang wrote: > // FFmpegBitstreamConverter implementation. Done. https://codereview.chromium.org/691233002/diff/80001/media/filters/ffmpeg_aac... media/filters/ffmpeg_aac_bitstream_converter.h:45: int number_of_frames_minus_one); On 2014/11/06 04:35:28, xhwang wrote: > nit: You can make this a file scoped static function in the cc file. The new > function can return the header, which will be more clear. You meant just move into unnamed namespace, correct? Did that, made pointer to hdr an arg so as to keep bool success status. Done. https://codereview.chromium.org/691233002/diff/80001/media/filters/ffmpeg_bit... File media/filters/ffmpeg_bitstream_converter.h (right): https://codereview.chromium.org/691233002/diff/80001/media/filters/ffmpeg_bit... media/filters/ffmpeg_bitstream_converter.h:21: virtual bool ConvertPacket(AVPacket* packet) = 0; On 2014/11/06 04:35:28, xhwang wrote: > What's the return value? Done. https://codereview.chromium.org/691233002/diff/80001/media/filters/ffmpeg_dem... File media/filters/ffmpeg_demuxer.h (right): https://codereview.chromium.org/691233002/diff/80001/media/filters/ffmpeg_dem... media/filters/ffmpeg_demuxer.h:53: class FFmpegBitstreamConverter; On 2014/11/06 04:35:28, xhwang wrote: > Fix order. Done. https://codereview.chromium.org/691233002/diff/80001/media/filters/ffmpeg_dem... media/filters/ffmpeg_demuxer.h:130: // Create new bitstream converter, destroying active converter if present On 2014/11/06 04:35:28, xhwang wrote: > Add period after "present". Done. https://codereview.chromium.org/691233002/diff/80001/media/filters/ffmpeg_h26... File media/filters/ffmpeg_h264_to_annex_b_bitstream_converter.h (right): https://codereview.chromium.org/691233002/diff/80001/media/filters/ffmpeg_h26... media/filters/ffmpeg_h264_to_annex_b_bitstream_converter.h:31: // Converts |packet| to H.264 Annex B bytestream format. This conversion is On 2014/11/06 04:35:28, xhwang wrote: > // FFmpegBitstreamConverter implementation. Done.
LGTM! Thanks for working on this! https://codereview.chromium.org/691233002/diff/100001/media/filters/ffmpeg_aa... File media/filters/ffmpeg_aac_bitstream_converter.cc (right): https://codereview.chromium.org/691233002/diff/100001/media/filters/ffmpeg_aa... media/filters/ffmpeg_aac_bitstream_converter.cc:14: // Create adts header and store in |hdr| Create_s_ https://codereview.chromium.org/691233002/diff/100001/media/filters/ffmpeg_aa... media/filters/ffmpeg_aac_bitstream_converter.cc:16: // Returns false if parameter values are for an unsupported configuration period @ end of setence https://codereview.chromium.org/691233002/diff/100001/media/filters/ffmpeg_aa... File media/filters/ffmpeg_aac_bitstream_converter_unittest.cc (right): https://codereview.chromium.org/691233002/diff/100001/media/filters/ffmpeg_aa... media/filters/ffmpeg_aac_bitstream_converter_unittest.cc:51: for(uint i = 0; i < sizeof(dummy_packet); i++) { s/uint/size_t https://codereview.chromium.org/691233002/diff/100001/media/filters/ffmpeg_aa... media/filters/ffmpeg_aac_bitstream_converter_unittest.cc:52: dummy_packet[i] = i+1; // Non-zero unique values This will overflow? If this is intentional, add a comment. https://codereview.chromium.org/691233002/diff/100001/media/filters/ffmpeg_aa... media/filters/ffmpeg_aac_bitstream_converter_unittest.cc:73: // Converter will be automatically cleaned up. Do you mean destructed? In that case, this comment is not necessary. https://codereview.chromium.org/691233002/diff/100001/media/filters/ffmpeg_aa... media/filters/ffmpeg_aac_bitstream_converter_unittest.cc:91: sizeof(dummy_packet)); fit in one line? https://codereview.chromium.org/691233002/diff/100001/media/filters/ffmpeg_aa... media/filters/ffmpeg_aac_bitstream_converter_unittest.cc:96: // Converted will be automatically cleaned up. ditto
https://codereview.chromium.org/691233002/diff/100001/media/filters/ffmpeg_aa... File media/filters/ffmpeg_aac_bitstream_converter.cc (right): https://codereview.chromium.org/691233002/diff/100001/media/filters/ffmpeg_aa... media/filters/ffmpeg_aac_bitstream_converter.cc:14: // Create adts header and store in |hdr| On 2014/11/07 04:24:09, xhwang wrote: > Create_s_ Done. https://codereview.chromium.org/691233002/diff/100001/media/filters/ffmpeg_aa... media/filters/ffmpeg_aac_bitstream_converter.cc:16: // Returns false if parameter values are for an unsupported configuration On 2014/11/07 04:24:09, xhwang wrote: > period @ end of setence Done. https://codereview.chromium.org/691233002/diff/100001/media/filters/ffmpeg_aa... File media/filters/ffmpeg_aac_bitstream_converter_unittest.cc (right): https://codereview.chromium.org/691233002/diff/100001/media/filters/ffmpeg_aa... media/filters/ffmpeg_aac_bitstream_converter_unittest.cc:51: for(uint i = 0; i < sizeof(dummy_packet); i++) { On 2014/11/07 04:24:09, xhwang wrote: > s/uint/size_t Done. https://codereview.chromium.org/691233002/diff/100001/media/filters/ffmpeg_aa... media/filters/ffmpeg_aac_bitstream_converter_unittest.cc:52: dummy_packet[i] = i+1; // Non-zero unique values On 2014/11/07 04:24:09, xhwang wrote: > This will overflow? If this is intentional, add a comment. Whoops wasn't paying attention, behavior is fine but reformatting to implement explicitly https://codereview.chromium.org/691233002/diff/100001/media/filters/ffmpeg_aa... media/filters/ffmpeg_aac_bitstream_converter_unittest.cc:73: // Converter will be automatically cleaned up. On 2014/11/07 04:24:09, xhwang wrote: > Do you mean destructed? In that case, this comment is not necessary. Done. https://codereview.chromium.org/691233002/diff/100001/media/filters/ffmpeg_aa... media/filters/ffmpeg_aac_bitstream_converter_unittest.cc:91: sizeof(dummy_packet)); On 2014/11/07 04:24:09, xhwang wrote: > fit in one line? Done. https://codereview.chromium.org/691233002/diff/100001/media/filters/ffmpeg_aa... media/filters/ffmpeg_aac_bitstream_converter_unittest.cc:96: // Converted will be automatically cleaned up. On 2014/11/07 04:24:09, xhwang wrote: > ditto Done.
The CQ bit was checked by kjoswiak@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/691233002/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_clang_dbg_recipe on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
Ok some things broke, and so I have a few questions. It's complaining about licensing for media/filters/ffmpeg_aac_bitstream_converter.cc Is this because of ffmpeg stuff, and how do I get it white listed like it wants (or what else should I change to remove error)? Second a windows build was complaining about dll export stuff, I think because h264 converter is MEDIA_EXPORT and the interface & aac converter are not. Is correct fix to just slap MEDIA_EXPORT on these classes, or should I do something else? (I don't really know a whole lot about dllexport and c++ in windows)
I think it's just tripping up on the word "Copyright" in the file. You can either delete the comment or figure out how to whitelist this file as a false positive.
Also yes, just media_export the aac converter.
The CQ bit was checked by kjoswiak@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/691233002/160001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_clang_dbg_recipe on tryserver.chromium.linux ()
The CQ bit was unchecked by kjoswiak@chromium.org
The CQ bit was checked by kjoswiak@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/691233002/180001
The CQ bit was unchecked by kjoswiak@chromium.org
Ok well I don't think its the use of word Copyright, will do some investigation and figure out what is actually setting this error off
On 2014/11/07 21:57:43, kjoswiak wrote: > Ok well I don't think its the use of word Copyright, will do some investigation > and figure out what is actually setting this error off Or wait, missed some lowercase copyrights. Still going to try to figure if I can check this locally
The CQ bit was checked by kjoswiak@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/691233002/200001
The CQ bit was unchecked by kjoswiak@chromium.org
The CQ bit was checked by kjoswiak@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/691233002/220001
Message was sent while issue was closed.
Committed patchset #12 (id:220001)
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/c04b6087078aa371c0407ed3c2b93b6375718f42 Cr-Commit-Position: refs/heads/master@{#303325}
Message was sent while issue was closed.
damienv@chromium.org changed reviewers: + damienv@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/691233002/diff/220001/media/filters/ffmpeg_aa... File media/filters/ffmpeg_aac_bitstream_converter.h (right): https://codereview.chromium.org/691233002/diff/220001/media/filters/ffmpeg_aa... media/filters/ffmpeg_aac_bitstream_converter.h:28: ~FFmpegAACBitstreamConverter() override; virtual ~FFmpegAACBitstreamConverter(); instead ? https://codereview.chromium.org/691233002/diff/220001/media/filters/ffmpeg_aa... media/filters/ffmpeg_aac_bitstream_converter.h:33: bool ConvertPacket(AVPacket* packet) override; Shouldn't this be: virtual ... ?
Message was sent while issue was closed.
On 2014/11/08 03:08:48, damienv1 wrote: > https://codereview.chromium.org/691233002/diff/220001/media/filters/ffmpeg_aa... > File media/filters/ffmpeg_aac_bitstream_converter.h (right): > > https://codereview.chromium.org/691233002/diff/220001/media/filters/ffmpeg_aa... > media/filters/ffmpeg_aac_bitstream_converter.h:28: > ~FFmpegAACBitstreamConverter() override; > virtual ~FFmpegAACBitstreamConverter(); instead ? > > https://codereview.chromium.org/691233002/diff/220001/media/filters/ffmpeg_aa... > media/filters/ffmpeg_aac_bitstream_converter.h:33: bool ConvertPacket(AVPacket* > packet) override; > Shouldn't this be: virtual ... ? damienv: These are new styles we are following :) http://chromium-cpp.appspot.com/ |
