Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
364823008:
Mpeg2TS - estimate duration for video frames. (Closed)
|
Created:
6 years, 5 months ago by damienv1 Modified:
6 years, 5 months ago CC:
chromium-reviews, feature-media-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
DescriptionMpeg2TS - estimate duration for video frames.
The updated frame processor assumes each frame is assigned
a positive duration.
Update the Mpeg2 TS stream parser to provide estimated duration
for video frames.
BUG=None
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=282585
Patch Set 1 #Patch Set 2 : #
Total comments: 5
Patch Set 3 : Delay frame buffer emission. #Patch Set 4 : Move the ES adapter out of the H264 ES parser. #Patch Set 5 : Remove uneeded code in Mp2tStreamParser. #Patch Set 6 : Additional cleanup. #
Total comments: 14
Patch Set 7 : Address CR comments. #
Total comments: 18
Patch Set 8 : Address CR comments. #
Total comments: 14
Patch Set 9 : Add an video ES adapter unit test. #
Total comments: 10
Patch Set 10 : Clean the ES adapter unit test. #Patch Set 11 : Fix MEDIA_EXPORT needed by the ES adapter unit test. #
Messages
Total messages: 24 (0 generated)
There is currently no way to get an accurate frame duration for Mpeg2 TS. This CL aims at providing an estimate of the frame duration, hoping this is enough to satisfy the upper frame processor (which do now require the duration for each frame).
https://codereview.chromium.org/364823008/diff/20001/media/formats/mp2t/es_pa... File media/formats/mp2t/es_parser_h264.cc (right): https://codereview.chromium.org/364823008/diff/20001/media/formats/mp2t/es_pa... media/formats/mp2t/es_parser_h264.cc:27: const base::TimeDelta kDefaultFrameDuration( nit: Please add a comment saying why you picked 25fps. https://codereview.chromium.org/364823008/diff/20001/media/formats/mp2t/es_pa... media/formats/mp2t/es_parser_h264.cc:30: const size_t kMaxPtsHistoryLength = 16; nit: I'm assuming this is derived based on the max number of B-frames that can appear after non-B-frames right? Please add a comment indicating where this number was derived from. https://codereview.chromium.org/364823008/diff/20001/media/formats/mp2t/es_pa... media/formats/mp2t/es_parser_h264.cc:269: ((kMaxPtsHistoryLength - pts_count) * kDefaultFrameDuration + I'm assuming that you are trying to smooth out the value for the first couple frames here right? Is that because you don't know how many B-frames there might be and you don't know if hierarchical B-frames are being used? Perhaps a comment? It seems like you shouldn't have to estimate if you delay emitting frames until you get to a non-B. I worry that incorrect estimates may cause problems downstream if the pts + duration overlaps another frame. https://codereview.chromium.org/364823008/diff/20001/media/formats/mp2t/es_pa... File media/formats/mp2t/es_parser_h264.h (right): https://codereview.chromium.org/364823008/diff/20001/media/formats/mp2t/es_pa... media/formats/mp2t/es_parser_h264.h:84: base::TimeDelta last_frame_duration_; Remove? This doesn't appear to be used anywhere.
https://codereview.chromium.org/364823008/diff/20001/media/formats/mp2t/es_pa... File media/formats/mp2t/es_parser_h264.cc (right): https://codereview.chromium.org/364823008/diff/20001/media/formats/mp2t/es_pa... media/formats/mp2t/es_parser_h264.cc:269: ((kMaxPtsHistoryLength - pts_count) * kDefaultFrameDuration + On 2014/07/03 00:50:58, acolwell wrote: > I'm assuming that you are trying to smooth out the value for the first couple > frames here right? Is that because you don't know how many B-frames there might > be and you don't know if hierarchical B-frames are being used? Perhaps a > comment? > > It seems like you shouldn't have to estimate if you delay emitting frames until > you get to a non-B. I worry that incorrect estimates may cause problems > downstream if the pts + duration overlaps another frame. > - In theory, to get the frame duration accurately, I would need to replicate exactly the H264 frame bumping process here (which would involve a latency for buffer emission, and that latency would be "optimal"). However, I don't think going that deep inside the H264 parsing is the right way either (at least, it is not inline with the spirit of a demuxer which should not have to parse ES streams in a first place). - Delaying the frame buffer emission with a constant latency (i.e. not going through the H264 frame bumping process) could be another way (also not as generic). Incorrect frame duration estimates could impact the upper layer only if the upper layer understands the decoding/rendering process (which shouldn't be the intent of MSE). I was not really in favor of introducing a latency in that part of the code (when you have a muxed audio/video stream, the impact is the same as the video was coming later into the stream: i.e. could make the stream become badly muxed stream from a buffer emission perspective). Let me think about it though.
Please take a look at the coarse structure of the code (i.e. whether the logic looks ok). Once this is ok. I'll cleanup the mp2_stream_parser.cc (remove the frame duplication logic when the stream does not start with a key frame).
Ping on this CL (this CL is one of the 2 CLs needed to fix some unit tests and Mpeg2 TS playback).
looks good to me. Just a few minor comments. I'll let Matt have the final say. https://codereview.chromium.org/364823008/diff/100001/media/formats/mp2t/es_a... File media/formats/mp2t/es_adapter_video.cc (right): https://codereview.chromium.org/364823008/diff/100001/media/formats/mp2t/es_a... media/formats/mp2t/es_adapter_video.cc:118: for (BufferQueue::const_iterator it = buffer_list_.begin(); nit: Might want to call out B-frames here since people might think the next frame should be sufficient and won't think about the IPB case. https://codereview.chromium.org/364823008/diff/100001/media/formats/mp2t/es_a... media/formats/mp2t/es_adapter_video.cc:127: for (std::list<base::TimeDelta>::const_iterator it = emitted_pts_.begin(); This is for the duration of the last B frame in IPBBB right? Perhaps add why we need to consider previous frames to the comment? https://codereview.chromium.org/364823008/diff/100001/media/formats/mp2t/es_a... File media/formats/mp2t/es_adapter_video.h (right): https://codereview.chromium.org/364823008/diff/100001/media/formats/mp2t/es_a... media/formats/mp2t/es_adapter_video.h:26: // - make sure the first video frame is a key frame. This comment is a little misleading. The FrameProcessor should be taking care of non-keyframes being passed in first. The code in this class is just replacing those non-keyframes with the first frame to avoid a hole from being created in the timeline if the video lags the audio significantly. Right? Does this still apply with the new FrameProcessor. https://codereview.chromium.org/364823008/diff/100001/media/formats/mp2t/es_a... media/formats/mp2t/es_adapter_video.h:38: void Flush(); nit: s/Flush/EmitAllPendingBuffers/? https://codereview.chromium.org/364823008/diff/100001/media/formats/mp2t/es_a... media/formats/mp2t/es_adapter_video.h:72: // Index of the first buffer in |buffer_list_|. nit: I think this comment may be missing something. I'd expect the "index of the first buffer in |buffer_list_|" to always be 0. s/Index/Global index/ or something like that since I believe this is tracking the that the first frame in buffer_list_ is the n'th buffer passed to this class. https://codereview.chromium.org/364823008/diff/100001/media/formats/mp2t/es_p... File media/formats/mp2t/es_parser_h264.cc (right): https://codereview.chromium.org/364823008/diff/100001/media/formats/mp2t/es_p... media/formats/mp2t/es_parser_h264.cc:31: emit_buffer_cb_(emit_buffer_cb), remove this and new_video_config_cb_ ? https://codereview.chromium.org/364823008/diff/100001/media/formats/mp2t/es_p... media/formats/mp2t/es_parser_h264.cc:93: es_adapter_.reset( Add a Reset/Clear method on the adapter so you don't need to cache these callbacks in this object?
https://codereview.chromium.org/364823008/diff/100001/media/formats/mp2t/es_a... File media/formats/mp2t/es_adapter_video.cc (right): https://codereview.chromium.org/364823008/diff/100001/media/formats/mp2t/es_a... media/formats/mp2t/es_adapter_video.cc:118: for (BufferQueue::const_iterator it = buffer_list_.begin(); On 2014/07/09 20:01:10, acolwell_OOO_7-10_and_7-11 wrote: > nit: Might want to call out B-frames here since people might think the next > frame should be sufficient and won't think about the IPB case. Done. https://codereview.chromium.org/364823008/diff/100001/media/formats/mp2t/es_a... media/formats/mp2t/es_adapter_video.cc:127: for (std::list<base::TimeDelta>::const_iterator it = emitted_pts_.begin(); On 2014/07/09 20:01:10, acolwell_OOO_7-10_and_7-11 wrote: > This is for the duration of the last B frame in IPBBB right? Perhaps add why we > need to consider previous frames to the comment? Done. https://codereview.chromium.org/364823008/diff/100001/media/formats/mp2t/es_a... File media/formats/mp2t/es_adapter_video.h (right): https://codereview.chromium.org/364823008/diff/100001/media/formats/mp2t/es_a... media/formats/mp2t/es_adapter_video.h:26: // - make sure the first video frame is a key frame. On 2014/07/09 20:01:10, acolwell_OOO_7-10_and_7-11 wrote: > This comment is a little misleading. The FrameProcessor should be taking care of > non-keyframes being passed in first. The code in this class is just replacing > those non-keyframes with the first frame to avoid a hole from being created in > the timeline if the video lags the audio significantly. Right? Does this still > apply with the new FrameProcessor. Right. The frame processor discards non key-frame. So the goal here is to replace non key frames by the first key frame to avoid a hole in the video source buffer (in that case, playback would stall). https://codereview.chromium.org/364823008/diff/100001/media/formats/mp2t/es_a... media/formats/mp2t/es_adapter_video.h:38: void Flush(); On 2014/07/09 20:01:11, acolwell_OOO_7-10_and_7-11 wrote: > nit: s/Flush/EmitAllPendingBuffers/? I am using Flush in EsParser. I'll keep the same naming here. https://codereview.chromium.org/364823008/diff/100001/media/formats/mp2t/es_a... media/formats/mp2t/es_adapter_video.h:72: // Index of the first buffer in |buffer_list_|. On 2014/07/09 20:01:10, acolwell_OOO_7-10_and_7-11 wrote: > nit: I think this comment may be missing something. I'd expect the "index of the > first buffer in |buffer_list_|" to always be 0. s/Index/Global index/ or > something like that since I believe this is tracking the that the first frame in > buffer_list_ is the n'th buffer passed to this class. Agree. The comment should be more accurate. This is an absolute index of the frame since the creation of the ES adapter. https://codereview.chromium.org/364823008/diff/100001/media/formats/mp2t/es_p... File media/formats/mp2t/es_parser_h264.cc (right): https://codereview.chromium.org/364823008/diff/100001/media/formats/mp2t/es_p... media/formats/mp2t/es_parser_h264.cc:31: emit_buffer_cb_(emit_buffer_cb), On 2014/07/09 20:01:11, acolwell_OOO_7-10_and_7-11 wrote: > remove this and new_video_config_cb_ ? Done. https://codereview.chromium.org/364823008/diff/100001/media/formats/mp2t/es_p... media/formats/mp2t/es_parser_h264.cc:93: es_adapter_.reset( On 2014/07/09 20:01:11, acolwell_OOO_7-10_and_7-11 wrote: > Add a Reset/Clear method on the adapter so you don't need to cache these > callbacks in this object? Done.
PTAL. Thanks !
First round of comments (questions, mostly): https://codereview.chromium.org/364823008/diff/120001/media/formats/mp2t/es_a... File media/formats/mp2t/es_adapter_video.cc (right): https://codereview.chromium.org/364823008/diff/120001/media/formats/mp2t/es_a... media/formats/mp2t/es_adapter_video.cc:186: nit: remove? I'm not sure rietveld lint is consistent with rest of style w.r.t. requiring a blank line prior to EOF. Or add empty line at end of the .h. https://codereview.chromium.org/364823008/diff/120001/media/formats/mp2t/es_a... File media/formats/mp2t/es_adapter_video.h (right): https://codereview.chromium.org/364823008/diff/120001/media/formats/mp2t/es_a... media/formats/mp2t/es_adapter_video.h:10: lint nit: Add #include for pair<> -- <utility> ? https://codereview.chromium.org/364823008/diff/120001/media/formats/mp2t/mp2t... File media/formats/mp2t/mp2t_stream_parser.cc (right): https://codereview.chromium.org/364823008/diff/120001/media/formats/mp2t/mp2t... media/formats/mp2t/mp2t_stream_parser.cc:515: DVLOG(1) << "Ignoring audio buffer with no corresponding audio config"; If there has been no init segment prior to media segment, shouldn't parse error occur? Should this clause trigger parse err? Furthermore, if there is a non-empty chain, but it's back() has no audio config, should we log or emit some kind of error indication here, too? https://codereview.chromium.org/364823008/diff/120001/media/formats/mp2t/mp2t... media/formats/mp2t/mp2t_stream_parser.cc:546: DVLOG(1) << "Ignoring video buffer with no corresponding video config"; If there has been no init segment prior to media segment, shouldn't parse error occur? Should this clause trigger parse err? Furthermore, if there is a non-empty chain, but it's back() has no video config, should we log or emit some kind of error indication here, too? https://codereview.chromium.org/364823008/diff/120001/media/formats/mp2t/mp2t... File media/formats/mp2t/mp2t_stream_parser_unittest.cc (right): https://codereview.chromium.org/364823008/diff/120001/media/formats/mp2t/mp2t... media/formats/mp2t/mp2t_stream_parser_unittest.cc:163: parser_->Flush(); Prior to Flush(), is there a non-flaky |video_frame_count_| we could verify? https://codereview.chromium.org/364823008/diff/120001/media/formats/mp2t/mp2t... media/formats/mp2t/mp2t_stream_parser_unittest.cc:171: parser_->Flush(); ditto https://codereview.chromium.org/364823008/diff/120001/media/formats/mp2t/mp2t... media/formats/mp2t/mp2t_stream_parser_unittest.cc:191: parser_->Flush(); ditto
https://codereview.chromium.org/364823008/diff/120001/media/formats/mp2t/es_a... File media/formats/mp2t/es_adapter_video.cc (right): https://codereview.chromium.org/364823008/diff/120001/media/formats/mp2t/es_a... media/formats/mp2t/es_adapter_video.cc:186: On 2014/07/10 17:57:23, wolenetz wrote: > nit: remove? I'm not sure rietveld lint is consistent with rest of style w.r.t. > requiring a blank line prior to EOF. Or add empty line at end of the .h. Done. https://codereview.chromium.org/364823008/diff/120001/media/formats/mp2t/es_a... File media/formats/mp2t/es_adapter_video.h (right): https://codereview.chromium.org/364823008/diff/120001/media/formats/mp2t/es_a... media/formats/mp2t/es_adapter_video.h:10: On 2014/07/10 17:57:23, wolenetz wrote: > lint nit: Add #include for pair<> -- <utility> ? Done. https://codereview.chromium.org/364823008/diff/120001/media/formats/mp2t/mp2t... File media/formats/mp2t/mp2t_stream_parser.cc (right): https://codereview.chromium.org/364823008/diff/120001/media/formats/mp2t/mp2t... media/formats/mp2t/mp2t_stream_parser.cc:515: DVLOG(1) << "Ignoring audio buffer with no corresponding audio config"; On 2014/07/10 17:57:23, wolenetz wrote: > If there has been no init segment prior to media segment, shouldn't parse error > occur? Should this clause trigger parse err? Furthermore, if there is a > non-empty chain, but it's back() has no audio config, should we log or emit some > kind of error indication here, too? True. With the updated code, there should always be a config before a frame is pushed: - was already the case for ADTS audio - is now the case of H264 as well: the ES adapter emits frame only after it gets the first config. Will replace DVLOG with NOTREACHED (I prefer it to a single DCHECK which would make "buffer_queue_chain_.back().audio_queue.push_back" crash in production. https://codereview.chromium.org/364823008/diff/120001/media/formats/mp2t/mp2t... media/formats/mp2t/mp2t_stream_parser.cc:546: DVLOG(1) << "Ignoring video buffer with no corresponding video config"; On 2014/07/10 17:57:23, wolenetz wrote: > If there has been no init segment prior to media segment, shouldn't parse error > occur? Should this clause trigger parse err? Furthermore, if there is a > non-empty chain, but it's back() has no video config, should we log or emit some > kind of error indication here, too? ditto. https://codereview.chromium.org/364823008/diff/120001/media/formats/mp2t/mp2t... File media/formats/mp2t/mp2t_stream_parser_unittest.cc (right): https://codereview.chromium.org/364823008/diff/120001/media/formats/mp2t/mp2t... media/formats/mp2t/mp2t_stream_parser_unittest.cc:163: parser_->Flush(); On 2014/07/10 17:57:23, wolenetz wrote: > Prior to Flush(), is there a non-flaky |video_frame_count_| we could verify? This all depends on the latency we introduce in the stream parser. Previously it was one frame. Now, it depends on the look ahead we use to calculate frame duration. I would prefer to avoid making some assumptions here which would depend on some implementation details. https://codereview.chromium.org/364823008/diff/120001/media/formats/mp2t/mp2t... media/formats/mp2t/mp2t_stream_parser_unittest.cc:171: parser_->Flush(); On 2014/07/10 17:57:23, wolenetz wrote: > ditto Same answer. May be I could consider sth like: EXPECT_GE(video_frame_count_, 82 - kMaxParserLatency); https://codereview.chromium.org/364823008/diff/120001/media/formats/mp2t/mp2t... media/formats/mp2t/mp2t_stream_parser_unittest.cc:191: parser_->Flush(); On 2014/07/10 17:57:23, wolenetz wrote: > ditto Done.
A couple more questions. Thanks! https://codereview.chromium.org/364823008/diff/120001/media/formats/mp2t/es_a... File media/formats/mp2t/es_adapter_video.cc (right): https://codereview.chromium.org/364823008/diff/120001/media/formats/mp2t/es_a... media/formats/mp2t/es_adapter_video.cc:81: stream_parser_buffer->GetDecodeTimestamp()); Does caller of OnNewBuffer ensure monotonically increasing DTS? https://codereview.chromium.org/364823008/diff/120001/media/formats/mp2t/es_a... media/formats/mp2t/es_adapter_video.cc:88: ReplaceDiscardedFrames(stream_parser_buffer); What would happen in the following sequence: A:Non-Keyframe (from some previous config unknown to this adapter) ConfigChange B:Keyframe ISTM that replacing A with B would result in eventually emitting keyframe B that may mismatch config meant for A. Do I misunderstand ProcessPendingBuffers' implementation? Alternatively, could ProcessPendingBuffers emit the new config for B prior to sending the two B's in a row?
https://codereview.chromium.org/364823008/diff/120001/media/formats/mp2t/es_a... File media/formats/mp2t/es_adapter_video.cc (right): https://codereview.chromium.org/364823008/diff/120001/media/formats/mp2t/es_a... media/formats/mp2t/es_adapter_video.cc:81: stream_parser_buffer->GetDecodeTimestamp()); On 2014/07/10 19:32:27, wolenetz wrote: > Does caller of OnNewBuffer ensure monotonically increasing DTS? No. The DTS comes directly from the stream (which in theory should have been encoded in such a way that DTS are increasing). https://codereview.chromium.org/364823008/diff/120001/media/formats/mp2t/es_a... media/formats/mp2t/es_adapter_video.cc:88: ReplaceDiscardedFrames(stream_parser_buffer); On 2014/07/10 19:32:27, wolenetz wrote: > What would happen in the following sequence: > A:Non-Keyframe (from some previous config unknown to this adapter) > ConfigChange > B:Keyframe > > ISTM that replacing A with B would result in eventually emitting keyframe B that > may mismatch config meant for A. > > Do I misunderstand ProcessPendingBuffers' implementation? > Alternatively, could ProcessPendingBuffers emit the new config for B prior to > sending the two B's in a row? Frames A that are replaced by B will be assigned the config that applies to B. When you check the code, the first config is always assigned global index = 0. (so it applies to the very first buffer emitted).
lgtm % nits and scherkus/acolwell/other OWNER's lgtm https://codereview.chromium.org/364823008/diff/140001/media/formats/mp2t/es_a... File media/formats/mp2t/es_adapter_video.cc (right): https://codereview.chromium.org/364823008/diff/140001/media/formats/mp2t/es_a... media/formats/mp2t/es_adapter_video.cc:114: buffer->set_duration(last_frame_duration_); nit: DVLOG this set_duration() please, and differentiate log from the other kind of set_duration, next. https://codereview.chromium.org/364823008/diff/140001/media/formats/mp2t/es_a... media/formats/mp2t/es_adapter_video.cc:116: buffer->set_duration(next_frame_pts - buffer->timestamp()); nit: DVLOG this set_duration() please
https://codereview.chromium.org/364823008/diff/140001/media/formats/mp2t/es_a... File media/formats/mp2t/es_adapter_video.cc (right): https://codereview.chromium.org/364823008/diff/140001/media/formats/mp2t/es_a... media/formats/mp2t/es_adapter_video.cc:17: static base::TimeDelta kDefaultFrameDuration( FYI this introduces a static initializer replace it with something like: static const int kDefaultFrameDurationMs = 40; https://codereview.chromium.org/364823008/diff/140001/media/formats/mp2t/es_a... media/formats/mp2t/es_adapter_video.cc:31: : new_video_config_cb_(new_video_config_cb), this code block should be +2 space indent https://codereview.chromium.org/364823008/diff/140001/media/formats/mp2t/es_a... File media/formats/mp2t/es_adapter_video.h (right): https://codereview.chromium.org/364823008/diff/140001/media/formats/mp2t/es_a... media/formats/mp2t/es_adapter_video.h:29: class EsAdapterVideo { what's our test coverage story for this class? https://codereview.chromium.org/364823008/diff/140001/media/formats/mp2t/es_p... File media/formats/mp2t/es_parser_h264.cc (right): https://codereview.chromium.org/364823008/diff/140001/media/formats/mp2t/es_p... media/formats/mp2t/es_parser_h264.cc:30: : es_adapter_(new EsAdapterVideo( +2 space indent for this block https://codereview.chromium.org/364823008/diff/140001/media/formats/mp2t/es_p... File media/formats/mp2t/es_parser_h264.h (right): https://codereview.chromium.org/364823008/diff/140001/media/formats/mp2t/es_p... media/formats/mp2t/es_parser_h264.h:77: scoped_ptr<EsAdapterVideo> es_adapter_; any reason why this needs to be a scoped_ptr<>? it looks like you construct one in the ctor and never reset() it
https://codereview.chromium.org/364823008/diff/140001/media/formats/mp2t/es_a... File media/formats/mp2t/es_adapter_video.cc (right): https://codereview.chromium.org/364823008/diff/140001/media/formats/mp2t/es_a... media/formats/mp2t/es_adapter_video.cc:17: static base::TimeDelta kDefaultFrameDuration( On 2014/07/10 20:45:34, scherkus wrote: > FYI this introduces a static initializer > > replace it with something like: > static const int kDefaultFrameDurationMs = 40; Done. https://codereview.chromium.org/364823008/diff/140001/media/formats/mp2t/es_a... media/formats/mp2t/es_adapter_video.cc:31: : new_video_config_cb_(new_video_config_cb), On 2014/07/10 20:45:34, scherkus wrote: > this code block should be +2 space indent Done. https://codereview.chromium.org/364823008/diff/140001/media/formats/mp2t/es_a... media/formats/mp2t/es_adapter_video.cc:114: buffer->set_duration(last_frame_duration_); On 2014/07/10 20:34:26, wolenetz wrote: > nit: DVLOG this set_duration() please, and differentiate log from the other kind > of set_duration, next. Done. https://codereview.chromium.org/364823008/diff/140001/media/formats/mp2t/es_a... media/formats/mp2t/es_adapter_video.cc:116: buffer->set_duration(next_frame_pts - buffer->timestamp()); On 2014/07/10 20:34:26, wolenetz wrote: > nit: DVLOG this set_duration() please Done. https://codereview.chromium.org/364823008/diff/140001/media/formats/mp2t/es_a... File media/formats/mp2t/es_adapter_video.h (right): https://codereview.chromium.org/364823008/diff/140001/media/formats/mp2t/es_a... media/formats/mp2t/es_adapter_video.h:29: class EsAdapterVideo { On 2014/07/10 20:45:34, scherkus wrote: > what's our test coverage story for this class? Dedicated unit test added. https://codereview.chromium.org/364823008/diff/140001/media/formats/mp2t/es_p... File media/formats/mp2t/es_parser_h264.cc (right): https://codereview.chromium.org/364823008/diff/140001/media/formats/mp2t/es_p... media/formats/mp2t/es_parser_h264.cc:30: : es_adapter_(new EsAdapterVideo( On 2014/07/10 20:45:34, scherkus wrote: > +2 space indent for this block Done. https://codereview.chromium.org/364823008/diff/140001/media/formats/mp2t/es_p... File media/formats/mp2t/es_parser_h264.h (right): https://codereview.chromium.org/364823008/diff/140001/media/formats/mp2t/es_p... media/formats/mp2t/es_parser_h264.h:77: scoped_ptr<EsAdapterVideo> es_adapter_; On 2014/07/10 20:45:34, scherkus wrote: > any reason why this needs to be a scoped_ptr<>? > > it looks like you construct one in the ctor and never reset() it I used to have a reset before, then changed it based on Aaron's comment. Will remove the scoped_ptr.
PTAL. Thanks for your feedback.
https://codereview.chromium.org/364823008/diff/160001/media/formats/mp2t/es_a... File media/formats/mp2t/es_adapter_video_unittest.cc (right): https://codereview.chromium.org/364823008/diff/160001/media/formats/mp2t/es_a... media/formats/mp2t/es_adapter_video_unittest.cc:95: EXPECT_EQ(expected_frame_duration_[buffer_count_], buffer->duration()); suggestion for improving the maintainability of this test... if/when this fails we're going to get an extremely uninformative failure along the lines of "5 != 7" but without any context (e.g., which buffer? what about the other ones?) a technique I like to use for these types of test to make them more human/logging friendly is to convert these results into a string, then use EXPECT_EQ() to compare strings for example: std::stringstream buffer_durations_; void OnNewBuffer() { buffer_durations_ << buffer->duration().InMilliseconds() << " "; } std::string EsAdapterVideoTest::RunDurationTest(...) { buffer_durations_.clear(); // ... do stuff std::string s = buffer_durations_.str(); base::TrimWhitespaceASCII(s, base::TRIM_ALL, &s); return s; } TEST_F(EsAdapterVideoTest, FrameDurationSimpleGop) { // PTS for a GOP without B frames - strictly increasing. int pts_ms[] = {30, 31, 33, 36, 40, 45, 51, 58}; EXPECT_EQ("1 2 3 4 5 6 7 7", RunDurationTest(pts_ms, arraysize(pts_ms)); }
https://codereview.chromium.org/364823008/diff/160001/media/formats/mp2t/es_a... File media/formats/mp2t/es_adapter_video_unittest.cc (right): https://codereview.chromium.org/364823008/diff/160001/media/formats/mp2t/es_a... media/formats/mp2t/es_adapter_video_unittest.cc:34: std::vector<scoped_refptr<StreamParserBuffer> > nit: use StreamParserBuffer::BufferQueue (a deque) instead of custom vector? https://codereview.chromium.org/364823008/diff/160001/media/formats/mp2t/es_a... media/formats/mp2t/es_adapter_video_unittest.cc:48: const int* timestamp_in_ms, size_t size) { nit: move to previous line and wrap after 1st param? https://codereview.chromium.org/364823008/diff/160001/media/formats/mp2t/es_a... media/formats/mp2t/es_adapter_video_unittest.cc:48: const int* timestamp_in_ms, size_t size) { nit: |size| could be misinterpreted as # of bytes pointed to by the *. use "count" or somesuch instead? https://codereview.chromium.org/364823008/diff/160001/media/formats/mp2t/es_a... media/formats/mp2t/es_adapter_video_unittest.cc:98: Thanks for adding test coverage - I would like to see some coverage of initial non-keyframes replaced by the first keyframe.
https://codereview.chromium.org/364823008/diff/160001/media/formats/mp2t/es_a... File media/formats/mp2t/es_adapter_video_unittest.cc (right): https://codereview.chromium.org/364823008/diff/160001/media/formats/mp2t/es_a... media/formats/mp2t/es_adapter_video_unittest.cc:34: std::vector<scoped_refptr<StreamParserBuffer> > On 2014/07/10 23:28:14, wolenetz wrote: > nit: use StreamParserBuffer::BufferQueue (a deque) instead of custom vector? Done. https://codereview.chromium.org/364823008/diff/160001/media/formats/mp2t/es_a... media/formats/mp2t/es_adapter_video_unittest.cc:48: const int* timestamp_in_ms, size_t size) { On 2014/07/10 23:28:13, wolenetz wrote: > nit: |size| could be misinterpreted as # of bytes pointed to by the *. use > "count" or somesuch instead? Done. https://codereview.chromium.org/364823008/diff/160001/media/formats/mp2t/es_a... media/formats/mp2t/es_adapter_video_unittest.cc:48: const int* timestamp_in_ms, size_t size) { On 2014/07/10 23:28:13, wolenetz wrote: > nit: |size| could be misinterpreted as # of bytes pointed to by the *. use > "count" or somesuch instead? Done. https://codereview.chromium.org/364823008/diff/160001/media/formats/mp2t/es_a... media/formats/mp2t/es_adapter_video_unittest.cc:95: EXPECT_EQ(expected_frame_duration_[buffer_count_], buffer->duration()); On 2014/07/10 23:13:58, scherkus wrote: > suggestion for improving the maintainability of this test... if/when this fails > we're going to get an extremely uninformative failure along the lines of "5 != > 7" but without any context (e.g., which buffer? what about the other ones?) > > a technique I like to use for these types of test to make them more > human/logging friendly is to convert these results into a string, then use > EXPECT_EQ() to compare strings > > for example: > > std::stringstream buffer_durations_; > > void OnNewBuffer() { > buffer_durations_ << buffer->duration().InMilliseconds() << " "; > } > > std::string EsAdapterVideoTest::RunDurationTest(...) { > buffer_durations_.clear(); > > // ... do stuff > > std::string s = buffer_durations_.str(); > base::TrimWhitespaceASCII(s, base::TRIM_ALL, &s); > return s; > } > > TEST_F(EsAdapterVideoTest, FrameDurationSimpleGop) { > // PTS for a GOP without B frames - strictly increasing. > int pts_ms[] = {30, 31, 33, 36, 40, 45, 51, 58}; > EXPECT_EQ("1 2 3 4 5 6 7 7", > RunDurationTest(pts_ms, arraysize(pts_ms)); > } Pretty convenient indeed. Thanks for the suggestion. https://codereview.chromium.org/364823008/diff/160001/media/formats/mp2t/es_a... media/formats/mp2t/es_adapter_video_unittest.cc:98: On 2014/07/10 23:28:13, wolenetz wrote: > Thanks for adding test coverage - I would like to see some coverage of initial > non-keyframes replaced by the first keyframe. Done.
lgtm -- thanks for writing tests! they're looking pretty sharp :)
Thanks for the reviews !
The CQ bit was checked by damienv@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/damienv@chromium.org/364823008/200001
Message was sent while issue was closed.
Change committed as 282585 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
