Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1162203009:
Access unit queue for MediaCodecPlayer (Closed)
|
Created:
5 years, 6 months ago by Tima Vaisburd Modified:
5 years, 6 months ago CC:
chromium-reviews, posciak+watch_chromium.org, feature-media-reviews_chromium.org, avayvod+watch_chromium.org, wjia+watch_chromium.org, mcasas+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAccess unit queue for MediaCodecPlayer
This is a prerequisite for MediaCodecPlayer (to be committed later).
The queue keeps access units that come from demuxer and makes them
available for the decoder. After consumption the unit at the front
is not removed immediately, providing some history to search back
for a key frame. The queue is thread safe.
For discussion see https://codereview.chromium.org/1128383003/
BUG=407577
Committed: https://crrev.com/3edc8c797947611b45dc30a9d5a34a6d467a43e6
Cr-Commit-Position: refs/heads/master@{#334294}
Patch Set 1 #
Total comments: 49
Patch Set 2 : Addressed comments, some questions. #
Total comments: 22
Patch Set 3 : Replaces scoped_ptr with raw pointers in std::list #
Total comments: 2
Patch Set 4 : Fixed leak in destructor #Patch Set 5 : Addressed Matt's comments #
Total comments: 16
Patch Set 6 : Addressed Matt's comments #
Total comments: 1
Patch Set 7 : Fixed one formatting issue. #
Messages
Total messages: 21 (3 generated)
timav@chromium.org changed reviewers: + qinmin@chromium.org, watk@chromium.org, wolenetz@chromium.org
Splitting AccessUnitQueue away from original https://codereview.chromium.org/1128383003/ as agreed with wolenetz@. Please take a look.
lgtm % nits https://codereview.chromium.org/1162203009/diff/1/media/base/android/access_u... File media/base/android/access_unit_queue_unittest.cc (right): https://codereview.chromium.org/1162203009/diff/1/media/base/android/access_u... media/base/android/access_unit_queue_unittest.cc:18: enum Flags {kNone = 0, kKeyFrame = 1, kEOS = 2, kConfig = 4}; nit: these flags are not used together, just enum Flags { kNone = 0, kKeyFrame, kEOS, kConfig }; also space after '{' and before '}' https://codereview.chromium.org/1162203009/diff/1/media/base/android/access_u... media/base/android/access_unit_queue_unittest.cc:74: {kNone, "0"}, nit: space before '}' and after '{', same below
Thanks for splitting this off. It makes the overall review more easy to fit into work schedules (and keeps the scope narrower for each to tease out any improper bindings/layering violations/etc) :) I'll review the rest of the tests in greater detail on next iteration of this CL. https://codereview.chromium.org/1162203009/diff/1/media/base/android/access_u... File media/base/android/access_unit_queue.cc (right): https://codereview.chromium.org/1162203009/diff/1/media/base/android/access_u... media/base/android/access_unit_queue.cc:25: // Verify the rule "one DemuxerConfigs for one AU with |kConfigChanged|". Like EOS (and more simple vs the current verification code), kConfigChanged must be the last unit in the chunk (if I read media_source_delegate.cc's OnBufferReady() correctly). https://codereview.chromium.org/1162203009/diff/1/media/base/android/access_u... media/base/android/access_unit_queue.cc:43: DCHECK(num_config_placeholders <= 1); num_config_placeholders is undefined here in non-DCHECK'ed builds. https://codereview.chromium.org/1162203009/diff/1/media/base/android/access_u... media/base/android/access_unit_queue.cc:102: // Search backwards in the current chunk only. This isn't clear from the .h comment. Also, why is there this restriction to current chunk only? https://codereview.chromium.org/1162203009/diff/1/media/base/android/access_u... media/base/android/access_unit_queue.cc:107: for (int i = index_in_chunk_; i >= 0; --i) { minor minor nit: On some platforms (like Windows), this assignment from size_t to int causes truncation, compile warning, and build error. Just FYI since this is Android, but please refrain from introducing this type of potential error in case this object were ever to be used on some other platform :) Also note that making "i" a "size_t" is also bad because it will then always be >= 0 :/ https://codereview.chromium.org/1162203009/diff/1/media/base/android/access_u... media/base/android/access_unit_queue.cc:131: info.front_unit = &(*current_chunk_)->access_units[index_in_chunk_]; Who owns the DemuxerData scoped pointer at this point? See my comment in .h: should we use a ScopedVector for chunks_ instead of std::list? Also, since this is essentially a "Peek" operation, perhaps the DemuxerData pointer should be refcounted since both ScopedVector and |info| reference it? https://codereview.chromium.org/1162203009/diff/1/media/base/android/access_u... media/base/android/access_unit_queue.cc:135: info.configs = &(*current_chunk_)->demuxer_configs[0]; Likewise: ownership? https://codereview.chromium.org/1162203009/diff/1/media/base/android/access_u... File media/base/android/access_unit_queue.h (right): https://codereview.chromium.org/1162203009/diff/1/media/base/android/access_u... media/base/android/access_unit_queue.h:19: // The data comes in the form of access units. Each access unit has a type, nit: s/, if/. If/ https://codereview.chromium.org/1162203009/diff/1/media/base/android/access_u... media/base/android/access_unit_queue.h:22: // The queue is accessed on the Media thread that puts the incoming data in and Are these assumptions checked in the .cc? If not (or if just commented in each method in the .cc, put those comments here for each method involved, and change this line to be more like "The queue should be accessed...". https://codereview.chromium.org/1162203009/diff/1/media/base/android/access_u... media/base/android/access_unit_queue.h:38: // Whether the queue contains End Of Stream. End of Stream is somewhat ephemeral: a web app could append some data, then mark the stream as ended, and after that point reopen the stream by various operations (like appending more data, removing some data, etc.). I'm curious: what would be the intended behavior of an AccessUnitQueue that had some AUs ending with an EOS unit, but then followed later by some more AUs and eventually another EOS unit? Would this even be possible, or is an AUQueue guaranteed to not contain more than one EOS unit due to the way it is populated and drained? Please add some comments at least around this. https://codereview.chromium.org/1162203009/diff/1/media/base/android/access_u... media/base/android/access_unit_queue.h:59: // access unit to it and returns true. Otherwise returns false. nit: Is the queue unmodified if the current "front position" is a keyframe (and I assume true would be returned in this case, too). In other words, is the look-back inclusive of the current front position? Please clarify in comments here. https://codereview.chromium.org/1162203009/diff/1/media/base/android/access_u... media/base/android/access_unit_queue.h:60: bool SkipToKeyFrame(); nit: s/SkipToKeyFrame()/RewindToLastKeyFrame()/ ? https://codereview.chromium.org/1162203009/diff/1/media/base/android/access_u... media/base/android/access_unit_queue.h:69: // Returns the amount of access units that has not been passed to decoder yet. nit: This class doesn't pass to a decoder, so shouldn't leak its users' logic. https://codereview.chromium.org/1162203009/diff/1/media/base/android/access_u... media/base/android/access_unit_queue.h:73: typedef std::list<scoped_ptr<DemuxerData>> DataChunkQueue; This doesn't look right to me. See "I want to use an STL container to hold pointers. Can I use smart pointers?" section at https://www.chromium.org/developers/smart-pointer-guidelines. https://codereview.chromium.org/1162203009/diff/1/media/base/android/access_u... File media/base/android/access_unit_queue_unittest.cc (right): https://codereview.chromium.org/1162203009/diff/1/media/base/android/access_u... media/base/android/access_unit_queue_unittest.cc:8: #define ARRAY_SIZE(x) (int)(sizeof(x)/sizeof(x[0])) nit ditto: size_t to int truncation. nit: add spaces around /, and parentheses around (int)(...) https://codereview.chromium.org/1162203009/diff/1/media/base/android/access_u... media/base/android/access_unit_queue_unittest.cc:18: enum Flags {kNone = 0, kKeyFrame = 1, kEOS = 2, kConfig = 4}; On 2015/06/08 19:39:38, qinmin wrote: > nit: these flags are not used together, just enum Flags { kNone = 0, kKeyFrame, > kEOS, kConfig }; > > also space after '{' and before '}' Suggestion: Please run this CL through clang format using "git cl format" to see what other formatting pieces we've missed during CR. Also, s/Flags/UnitTypes/ (or UnitStatus ?) and s/kNone/kNormal/ ? https://codereview.chromium.org/1162203009/diff/1/media/base/android/access_u... media/base/android/access_unit_queue_unittest.cc:20: int flags; nit: s/flags/unit_type/ (or unit_status ?) ? https://codereview.chromium.org/1162203009/diff/1/media/base/android/access_u... media/base/android/access_unit_queue_unittest.cc:30: result.type = DemuxerStream::AUDIO; // assign a valid type nit: Here and elsewhere: two spaces prior to same-line comment https://codereview.chromium.org/1162203009/diff/1/media/base/android/access_u... media/base/android/access_unit_queue_unittest.cc:55: while (0) nit: put while on previous line
Matt, thank you for the thorough review! I addressed your comments except for the underlying data structure (list<scoped_ptr<T>>) where I asked questions. Please take a look. https://codereview.chromium.org/1162203009/diff/1/media/base/android/access_u... File media/base/android/access_unit_queue.cc (right): https://codereview.chromium.org/1162203009/diff/1/media/base/android/access_u... media/base/android/access_unit_queue.cc:25: // Verify the rule "one DemuxerConfigs for one AU with |kConfigChanged|". On 2015/06/08 21:37:08, wolenetz wrote: > Like EOS (and more simple vs the current verification code), kConfigChanged must > be the last unit in the chunk (if I read media_source_delegate.cc's > OnBufferReady() correctly). Changed the comment and simplified the check. https://codereview.chromium.org/1162203009/diff/1/media/base/android/access_u... media/base/android/access_unit_queue.cc:43: DCHECK(num_config_placeholders <= 1); On 2015/06/08 21:37:09, wolenetz wrote: > num_config_placeholders is undefined here in non-DCHECK'ed builds. Removed since the checking code is simplified. https://codereview.chromium.org/1162203009/diff/1/media/base/android/access_u... media/base/android/access_unit_queue.cc:102: // Search backwards in the current chunk only. On 2015/06/08 21:37:08, wolenetz wrote: > This isn't clear from the .h comment. Also, why is there this restriction to > current chunk only? Updated the comment in .h. I searched in the current chunk only because currently we do not keep the old chunk (i.e. the history is just up to 4 units of the current chunk). With this patch I removed the assumption that there is one chunk only and extended the search back. I also set the history size (in chunks) controllable. I do not know, however, whether the increased complexity worth it. Please take a look. https://codereview.chromium.org/1162203009/diff/1/media/base/android/access_u... media/base/android/access_unit_queue.cc:107: for (int i = index_in_chunk_; i >= 0; --i) { On 2015/06/08 21:37:09, wolenetz wrote: > minor minor nit: On some platforms (like Windows), this assignment from size_t > to int causes truncation, compile warning, and build error. Just FYI since this > is Android, but please refrain from introducing this type of potential error in > case this object were ever to be used on some other platform :) > Also note that making "i" a "size_t" is also bad because it will then always be > >= 0 :/ I did not find an elegant solution, just put a cast to int. https://codereview.chromium.org/1162203009/diff/1/media/base/android/access_u... media/base/android/access_unit_queue.cc:131: info.front_unit = &(*current_chunk_)->access_units[index_in_chunk_]; On 2015/06/08 21:37:08, wolenetz wrote: > Who owns the DemuxerData scoped pointer at this point? See my comment in .h: The container remains to be the owner. I wanted to return a non-owning pointer while having container to own the object. I brought my consideration why I think it's possible to use such container in h: But if you say that this kind of interface is ugly, I'd humbly agree. > Also, since this is essentially a "Peek" operation, perhaps the DemuxerData > pointer should be refcounted since both ScopedVector and |info| reference it? I wanted to avoid extra locks. Maybe I shouldn't have. > should we use a ScopedVector for chunks_ instead of std::list? I used a list because it does not invalidate the iterator to the current chunk. Will scoped_refptr in the container and Info eliminate your concerns? https://codereview.chromium.org/1162203009/diff/1/media/base/android/access_u... media/base/android/access_unit_queue.cc:135: info.configs = &(*current_chunk_)->demuxer_configs[0]; On 2015/06/08 21:37:09, wolenetz wrote: > Likewise: ownership? Same as above https://codereview.chromium.org/1162203009/diff/1/media/base/android/access_u... File media/base/android/access_unit_queue.h (right): https://codereview.chromium.org/1162203009/diff/1/media/base/android/access_u... media/base/android/access_unit_queue.h:19: // The data comes in the form of access units. Each access unit has a type, On 2015/06/08 21:37:09, wolenetz wrote: > nit: s/, if/. If/ Done. https://codereview.chromium.org/1162203009/diff/1/media/base/android/access_u... media/base/android/access_unit_queue.h:22: // The queue is accessed on the Media thread that puts the incoming data in and On 2015/06/08 21:37:09, wolenetz wrote: > Are these assumptions checked in the .cc? If not (or if just commented in each > method in the .cc, put those comments here for each method involved, and change > this line to be more like "The queue should be accessed...". No, the assumptions are not checked, and I changed the comment to "should be". As far as other methods, however, all public methods except NumChunksForTesting() are thread safe and can be accessed from any thread. The comment at the top was more about the general purpose of this class. https://codereview.chromium.org/1162203009/diff/1/media/base/android/access_u... media/base/android/access_unit_queue.h:38: // Whether the queue contains End Of Stream. > what would be the intended behavior of an AccessUnitQueue that had some AUs > ending with an EOS unit, but then followed later by some more AUs and eventually > another EOS unit? Would this even be possible If you consider this class separately, I do not see what prevents it. I think, theoretically, yes. > or is an AUQueue guaranteed to not contain more than one EOS unit due to > the way it is populated and drained? The second and later EOS should not have any effect: after consuming first EOS from the queue decoder should not call Advance() unless there is a Flush(). I guess the best way would be blocking the input if EOS is pushed into the queue. Just double checking: is more data after EOS the real option (i.e. AU, AU, EOS, AU, AU, ... )? > https://codereview.chromium.org/1162203009/diff/1/media/base/android/access_u... media/base/android/access_unit_queue.h:59: // access unit to it and returns true. Otherwise returns false. On 2015/06/08 21:37:09, wolenetz wrote: > nit: Is the queue unmodified if the current "front position" is a keyframe (and > I assume true would be returned in this case, too). In other words, is the > look-back inclusive of the current front position? Please clarify in comments > here. Inclusive. I changed the comment. https://codereview.chromium.org/1162203009/diff/1/media/base/android/access_u... media/base/android/access_unit_queue.h:60: bool SkipToKeyFrame(); On 2015/06/08 21:37:09, wolenetz wrote: > nit: s/SkipToKeyFrame()/RewindToLastKeyFrame()/ ? Done. https://codereview.chromium.org/1162203009/diff/1/media/base/android/access_u... media/base/android/access_unit_queue.h:69: // Returns the amount of access units that has not been passed to decoder yet. On 2015/06/08 21:37:09, wolenetz wrote: > nit: This class doesn't pass to a decoder, so shouldn't leak its users' logic. Done. https://codereview.chromium.org/1162203009/diff/1/media/base/android/access_u... media/base/android/access_unit_queue.h:73: typedef std::list<scoped_ptr<DemuxerData>> DataChunkQueue; On 2015/06/08 21:37:09, wolenetz wrote: > This doesn't look right to me. See "I want to use an STL container to hold > pointers. Can I use smart pointers?" section at > https://www.chromium.org/developers/smart-pointer-guidelines. Interesting. There is a lot of examples of using unique_ptr in STL containers, including Google's own C++11 course. The link refers to "Effective STL" for the rational. As far as I can get the rational is that copy ctor does not have copy semantics and thus algorithms have undefined behavior. Again, only as far as I understand, by not having copy ctor the unique_ptr will only allow algorithms that do not expect the copy semantics. I thought scoped_ptr can be used as unique_ptr replacement. https://codereview.chromium.org/1162203009/diff/1/media/base/android/access_u... File media/base/android/access_unit_queue_unittest.cc (right): https://codereview.chromium.org/1162203009/diff/1/media/base/android/access_u... media/base/android/access_unit_queue_unittest.cc:8: #define ARRAY_SIZE(x) (int)(sizeof(x)/sizeof(x[0])) On 2015/06/08 21:37:09, wolenetz wrote: > nit ditto: size_t to int truncation. > nit: add spaces around /, and parentheses around (int)(...) Done. https://codereview.chromium.org/1162203009/diff/1/media/base/android/access_u... media/base/android/access_unit_queue_unittest.cc:18: enum Flags {kNone = 0, kKeyFrame = 1, kEOS = 2, kConfig = 4}; On 2015/06/08 19:39:38, qinmin wrote: > nit: these flags are not used together, just enum Flags { kNone = 0, kKeyFrame, > kEOS, kConfig }; > > also space after '{' and before '}' Done. https://codereview.chromium.org/1162203009/diff/1/media/base/android/access_u... media/base/android/access_unit_queue_unittest.cc:18: enum Flags {kNone = 0, kKeyFrame = 1, kEOS = 2, kConfig = 4}; On 2015/06/08 21:37:09, wolenetz wrote: > On 2015/06/08 19:39:38, qinmin wrote: > > nit: these flags are not used together, just enum Flags { kNone = 0, > kKeyFrame, > > kEOS, kConfig }; > > > > also space after '{' and before '}' > > Suggestion: Please run this CL through clang format using "git cl format" to see > what other formatting pieces we've missed during CR. > Also, s/Flags/UnitTypes/ (or UnitStatus ?) and s/kNone/kNormal/ ? Done renaming and ran through CL. After that I modified the way description arrays are defined, I did not like what cl format did with them. https://codereview.chromium.org/1162203009/diff/1/media/base/android/access_u... media/base/android/access_unit_queue_unittest.cc:20: int flags; On 2015/06/08 21:37:09, wolenetz wrote: > nit: s/flags/unit_type/ (or unit_status ?) ? Done. https://codereview.chromium.org/1162203009/diff/1/media/base/android/access_u... media/base/android/access_unit_queue_unittest.cc:30: result.type = DemuxerStream::AUDIO; // assign a valid type On 2015/06/08 21:37:09, wolenetz wrote: > nit: Here and elsewhere: two spaces prior to same-line comment Done. https://codereview.chromium.org/1162203009/diff/1/media/base/android/access_u... media/base/android/access_unit_queue_unittest.cc:55: while (0) On 2015/06/08 21:37:09, wolenetz wrote: > nit: put while on previous line Done. https://codereview.chromium.org/1162203009/diff/1/media/base/android/access_u... media/base/android/access_unit_queue_unittest.cc:74: {kNone, "0"}, On 2015/06/08 19:39:38, qinmin wrote: > nit: space before '}' and after '{', same below cl format removed these spaces.
Following the discussion we has with Matt offline, I changed the list to hold raw pointers. The list still owns them.
https://codereview.chromium.org/1162203009/diff/40001/media/base/android/acce... File media/base/android/access_unit_queue.cc (right): https://codereview.chromium.org/1162203009/diff/40001/media/base/android/acce... media/base/android/access_unit_queue.cc:28: } shouldn't we call STLDeleteContainerPointers() here?
Comments on most of just patch set 2 (some of the latter tests I didn't have time to review yet, nor patch set 3). I'll do more review later today after some other planned training is done. https://codereview.chromium.org/1162203009/diff/1/media/base/android/access_u... File media/base/android/access_unit_queue.cc (right): https://codereview.chromium.org/1162203009/diff/1/media/base/android/access_u... media/base/android/access_unit_queue.cc:102: // Search backwards in the current chunk only. On 2015/06/09 21:29:21, Tima Vaisburd wrote: > On 2015/06/08 21:37:08, wolenetz wrote: > > This isn't clear from the .h comment. Also, why is there this restriction to > > current chunk only? > > Updated the comment in .h. > > I searched in the current chunk only because currently we do not keep the old > chunk (i.e. the history is just up to 4 units of the current chunk). > With this patch I removed the assumption that there is one chunk only and > extended the search back. I also set the history size (in chunks) controllable. > I do not know, however, whether the increased complexity worth it. Please take a > look. I tend to agree that increased complexity might not be worth it (see my first comment). Some comment in Advance() about not keeping the advanced-from-chunk if the advance moved it to the next chunk would be useful if dropping the history piece. Sorry if my original comment churned you on this. "in the current chunk only" triggered me to wonder why, and if the API were meant to have 0 chunk history, then that comment makes more sense. However, if trying to avoid BrowserSeeks (up to some max double-caching-with chunkdemuxer threshold), then history might make sense. https://codereview.chromium.org/1162203009/diff/1/media/base/android/access_u... media/base/android/access_unit_queue.cc:131: info.front_unit = &(*current_chunk_)->access_units[index_in_chunk_]; On 2015/06/09 21:29:21, Tima Vaisburd wrote: > On 2015/06/08 21:37:08, wolenetz wrote: > > Who owns the DemuxerData scoped pointer at this point? See my comment in .h: > > The container remains to be the owner. I wanted to return a non-owning pointer > while having container to own the object. I brought my consideration why I think > it's possible to use such container in h: > > But if you say that this kind of interface is ugly, I'd humbly agree. > > > Also, since this is essentially a "Peek" operation, perhaps the DemuxerData > > pointer should be refcounted since both ScopedVector and |info| reference it? > > I wanted to avoid extra locks. Maybe I shouldn't have. > > > should we use a ScopedVector for chunks_ instead of std::list? > > I used a list because it does not invalidate the iterator to the current chunk. > Will scoped_refptr in the container and Info eliminate your concerns? > See my other comments. https://codereview.chromium.org/1162203009/diff/1/media/base/android/access_u... File media/base/android/access_unit_queue.h (right): https://codereview.chromium.org/1162203009/diff/1/media/base/android/access_u... media/base/android/access_unit_queue.h:22: // The queue is accessed on the Media thread that puts the incoming data in and On 2015/06/09 21:29:21, Tima Vaisburd wrote: > On 2015/06/08 21:37:09, wolenetz wrote: > > Are these assumptions checked in the .cc? If not (or if just commented in each > > method in the .cc, put those comments here for each method involved, and > change > > this line to be more like "The queue should be accessed...". > > No, the assumptions are not checked, and I changed the comment to "should be". > As far as other methods, however, all public methods except > NumChunksForTesting() are thread safe and can be accessed from any thread. The > comment at the top was more about the general purpose of this class. Acknowledged. https://codereview.chromium.org/1162203009/diff/1/media/base/android/access_u... media/base/android/access_unit_queue.h:38: // Whether the queue contains End Of Stream. On 2015/06/09 21:29:21, Tima Vaisburd wrote: > > what would be the intended behavior of an AccessUnitQueue that had some AUs > > ending with an EOS unit, but then followed later by some more AUs and > eventually > > another EOS unit? Would this even be possible > > If you consider this class separately, I do not see what prevents it. I think, > theoretically, yes. > > > or is an AUQueue guaranteed to not contain more than one EOS unit due to > > the way it is populated and drained? > > The second and later EOS should not have any effect: after consuming first EOS > from the queue decoder should not call Advance() unless there is a Flush(). I > guess the best way would be blocking the input if EOS is pushed into the queue. > > Just double checking: is more data after EOS the real option (i.e. AU, AU, EOS, > AU, AU, ... )? > > > Blocking any input after EOS (with some DCHECK or CHECK(!has_eos) on input), and caller using Flush() to clear all including any EOS sgtm. Essentially, guarantee that, if has_eos is true, then the last AU in the queue is that EOS AU. More data after EOS could happen due to web app behavior: it's entirely allowed for an MSE web app to append some data, then call MediaSource.endOfStream() (without any optional error argument), the player to play to 'ended' state, then the app to either seek or append more data. In the seek case, the AUQueue should be flushed and repopulated by reading from the demuxer. In either case, once an EOS has been read from the AUqueue, the caller can track the right behavior and remove the EOS or flush the queue. https://codereview.chromium.org/1162203009/diff/1/media/base/android/access_u... media/base/android/access_unit_queue.h:73: typedef std::list<scoped_ptr<DemuxerData>> DataChunkQueue; On 2015/06/09 21:29:21, Tima Vaisburd wrote: > On 2015/06/08 21:37:09, wolenetz wrote: > > This doesn't look right to me. See "I want to use an STL container to hold > > pointers. Can I use smart pointers?" section at > > https://www.chromium.org/developers/smart-pointer-guidelines. > > Interesting. There is a lot of examples of using unique_ptr in STL containers, > including Google's own C++11 course. The link refers to "Effective STL" for the > rational. As far as I can get the rational is that copy ctor does not have copy > semantics and thus algorithms have undefined behavior. Again, only as far as I > understand, by not having copy ctor the unique_ptr will only allow algorithms > that do not expect the copy semantics. I thought scoped_ptr can be used as > unique_ptr replacement. I don't think scoped_ptr can be a unique_ptr replacement. STL container internals don't use scoped_ptr.Pass(), TakesOwnership(), etc, when they move things around in memory. I could be wrong, but I defer to the referenced guidelines. If ScopedVector is insufficient, then std::list<raw pointer> or std::list<ScopedRefPtr..>. Use the former ONLY if there is guaranteed to be exactly one user of the pointed-to-object at a time, and document it well. See stl_utils for some tools if going that route. ScopedRefPtr is for when you're not sure how many owners there might be at a time, and hopefully we don't need that here (though we cannot guarantee exactly one caller of the GetInfo(), so at least document that very well if going the raw ptr route.) https://codereview.chromium.org/1162203009/diff/20001/media/base/android/acce... File media/base/android/access_unit_queue.cc (right): https://codereview.chromium.org/1162203009/diff/20001/media/base/android/acce... media/base/android/access_unit_queue.cc:21: history_chunks_amount_(kDefaultHistoryChunksAmount), aside: Do we ever need this? If not, always 0? If just being generic, I'm fine (have no strong feeling). My suspicion is that caching lookback prior to current chunk is doubly-caching what chunk_demuxer does. ANyway, just an aside, no need for this to change here, though watch out for double-caching (BrowserSeek already does the necessary for us). https://codereview.chromium.org/1162203009/diff/20001/media/base/android/acce... media/base/android/access_unit_queue.cc:34: // If there is an AU with |kConfigChanged| status, it must me the last nits: s/ / / and s/me/be/ https://codereview.chromium.org/1162203009/diff/20001/media/base/android/acce... media/base/android/access_unit_queue.cc:35: // AU in the chunk and the data should have exactly one correslonging nit: correslonging :) https://codereview.chromium.org/1162203009/diff/20001/media/base/android/acce... media/base/android/access_unit_queue.cc:85: if ((*current_chunk_)->access_units.size() <= index_in_chunk_) { nit: change the condition and return early if we're still in the middle of the current_chunk_ https://codereview.chromium.org/1162203009/diff/20001/media/base/android/acce... media/base/android/access_unit_queue.cc:92: // std::distance() and std::advance() do not work efficiently with std::list, aside: I suppose we could cache the queue size ourselves if we needed perf gain. https://codereview.chromium.org/1162203009/diff/20001/media/base/android/acce... media/base/android/access_unit_queue.cc:137: current_chunk_ = --rchunk.base(); would be good to test this. https://codereview.chromium.org/1162203009/diff/20001/media/base/android/acce... File media/base/android/access_unit_queue.h (right): https://codereview.chromium.org/1162203009/diff/20001/media/base/android/acce... media/base/android/access_unit_queue.h:66: // For unit tests only. nit: mark as not thread-safe in comment? https://codereview.chromium.org/1162203009/diff/20001/media/base/android/acce... media/base/android/access_unit_queue.h:72: // incuding current. Logically these are units that has not been consumed. nit: s/has/have/ https://codereview.chromium.org/1162203009/diff/20001/media/base/android/acce... File media/base/android/access_unit_queue_unittest.cc (right): https://codereview.chromium.org/1162203009/diff/20001/media/base/android/acce... media/base/android/access_unit_queue_unittest.cc:8: #define ARRAY_SIZE(x) ((sizeof(x) / sizeof(x[0]))) nit: either remove extra (), or put (some type like int) after initial (. https://codereview.chromium.org/1162203009/diff/20001/media/base/android/acce... media/base/android/access_unit_queue_unittest.cc:106: EXPECT_NE(nullptr, info.front_unit); nit: s/EXPECT/ASSERT to early fail instead of deref null crash on next line https://codereview.chromium.org/1162203009/diff/20001/media/base/android/acce... media/base/android/access_unit_queue_unittest.cc:135: // 5 is an arbitrary number, it implies that we keep 4 chunks of history. Does this need to be arbitrary? If the code changes, we should change the test. We can set the history and verify exactly that it is observed in test.
https://codereview.chromium.org/1162203009/diff/40001/media/base/android/acce... File media/base/android/access_unit_queue.cc (right): https://codereview.chromium.org/1162203009/diff/40001/media/base/android/acce... media/base/android/access_unit_queue.cc:28: } On 2015/06/11 19:12:13, qinmin wrote: > shouldn't we call STLDeleteContainerPointers() here? Thank you!
https://codereview.chromium.org/1162203009/diff/1/media/base/android/access_u... File media/base/android/access_unit_queue.cc (right): https://codereview.chromium.org/1162203009/diff/1/media/base/android/access_u... media/base/android/access_unit_queue.cc:102: // Search backwards in the current chunk only. On 2015/06/11 19:21:20, wolenetz wrote: > Sorry if my original comment churned you on this. That's no problem in general, and I went forward without asking in particular. But some of your other comments leave an impression that it's actually ok to keep. Please clarify, until then I'll keep it. > However, if trying to avoid BrowserSeeks (up to some max double-caching-with > chunkdemuxer threshold), then history might make sense. Exactly, I thought we might want to play with the number. https://codereview.chromium.org/1162203009/diff/1/media/base/android/access_u... File media/base/android/access_unit_queue.h (right): https://codereview.chromium.org/1162203009/diff/1/media/base/android/access_u... media/base/android/access_unit_queue.h:38: // Whether the queue contains End Of Stream. On 2015/06/11 19:21:20, wolenetz wrote: > Blocking any input after EOS (with some DCHECK or CHECK(!has_eos) on input), and > caller using Flush() to clear all including any EOS sgtm. I did the block. I'm not sure what to DCHECK though: as far as I understood, the data after first EOS is a valid input. > Essentially, guarantee > that, if has_eos is true, then the last AU in the queue is that EOS AU. It can be empty if we consumed everything. The Flush() works as you describe as far as I can tell. https://codereview.chromium.org/1162203009/diff/1/media/base/android/access_u... media/base/android/access_unit_queue.h:73: typedef std::list<scoped_ptr<DemuxerData>> DataChunkQueue; On 2015/06/11 19:21:20, wolenetz wrote: > On 2015/06/09 21:29:21, Tima Vaisburd wrote: > > On 2015/06/08 21:37:09, wolenetz wrote: > > > This doesn't look right to me. See "I want to use an STL container to hold > > > pointers. Can I use smart pointers?" section at > > > https://www.chromium.org/developers/smart-pointer-guidelines. > > > > Interesting. There is a lot of examples of using unique_ptr in STL containers, > > including Google's own C++11 course. The link refers to "Effective STL" for > the > > rational. As far as I can get the rational is that copy ctor does not have > copy > > semantics and thus algorithms have undefined behavior. Again, only as far as I > > understand, by not having copy ctor the unique_ptr will only allow algorithms > > that do not expect the copy semantics. I thought scoped_ptr can be used as > > unique_ptr replacement. > > I don't think scoped_ptr can be a unique_ptr replacement. STL container > internals don't use scoped_ptr.Pass(), TakesOwnership(), etc, when they move > things around in memory. I could be wrong, but I defer to the referenced > guidelines. > > If ScopedVector is insufficient, then std::list<raw pointer> or > std::list<ScopedRefPtr..>. Use the former ONLY if there is guaranteed to be > exactly one user of the pointed-to-object at a time, and document it well. See > stl_utils for some tools if going that route. ScopedRefPtr is for when you're > not sure how many owners there might be at a time, and hopefully we don't need > that here (though we cannot guarantee exactly one caller of the GetInfo(), so at > least document that very well if going the raw ptr route.) I went with raw pointer, the decoder seems to provide the guarantees you've mentioned. I added more documentation in the header. https://codereview.chromium.org/1162203009/diff/20001/media/base/android/acce... File media/base/android/access_unit_queue.cc (right): https://codereview.chromium.org/1162203009/diff/20001/media/base/android/acce... media/base/android/access_unit_queue.cc:34: // If there is an AU with |kConfigChanged| status, it must me the last On 2015/06/11 19:21:20, wolenetz wrote: > nits: s/ / / and s/me/be/ Done. https://codereview.chromium.org/1162203009/diff/20001/media/base/android/acce... media/base/android/access_unit_queue.cc:35: // AU in the chunk and the data should have exactly one correslonging On 2015/06/11 19:21:20, wolenetz wrote: > nit: correslonging :) Done. https://codereview.chromium.org/1162203009/diff/20001/media/base/android/acce... media/base/android/access_unit_queue.cc:85: if ((*current_chunk_)->access_units.size() <= index_in_chunk_) { On 2015/06/11 19:21:20, wolenetz wrote: > nit: change the condition and return early if we're still in the middle of the > current_chunk_ Done. https://codereview.chromium.org/1162203009/diff/20001/media/base/android/acce... media/base/android/access_unit_queue.cc:92: // std::distance() and std::advance() do not work efficiently with std::list, On 2015/06/11 19:21:21, wolenetz wrote: > aside: I suppose we could cache the queue size ourselves if we needed perf gain. Acknowledged. https://codereview.chromium.org/1162203009/diff/20001/media/base/android/acce... media/base/android/access_unit_queue.cc:137: current_chunk_ = --rchunk.base(); On 2015/06/11 19:21:20, wolenetz wrote: > would be good to test this. I added a new test KeyFrameWithLongHistory just because of this code. It should cover. https://codereview.chromium.org/1162203009/diff/20001/media/base/android/acce... File media/base/android/access_unit_queue.h (right): https://codereview.chromium.org/1162203009/diff/20001/media/base/android/acce... media/base/android/access_unit_queue.h:66: // For unit tests only. On 2015/06/11 19:21:21, wolenetz wrote: > nit: mark as not thread-safe in comment? Done. https://codereview.chromium.org/1162203009/diff/20001/media/base/android/acce... media/base/android/access_unit_queue.h:72: // incuding current. Logically these are units that has not been consumed. On 2015/06/11 19:21:21, wolenetz wrote: > nit: s/has/have/ Done. https://codereview.chromium.org/1162203009/diff/20001/media/base/android/acce... File media/base/android/access_unit_queue_unittest.cc (right): https://codereview.chromium.org/1162203009/diff/20001/media/base/android/acce... media/base/android/access_unit_queue_unittest.cc:8: #define ARRAY_SIZE(x) ((sizeof(x) / sizeof(x[0]))) On 2015/06/11 19:21:21, wolenetz wrote: > nit: either remove extra (), or put (some type like int) after initial (. Removed extra () https://codereview.chromium.org/1162203009/diff/20001/media/base/android/acce... media/base/android/access_unit_queue_unittest.cc:106: EXPECT_NE(nullptr, info.front_unit); On 2015/06/11 19:21:21, wolenetz wrote: > nit: s/EXPECT/ASSERT to early fail instead of deref null crash on next line Done. https://codereview.chromium.org/1162203009/diff/20001/media/base/android/acce... media/base/android/access_unit_queue_unittest.cc:135: // 5 is an arbitrary number, it implies that we keep 4 chunks of history. On 2015/06/11 19:21:21, wolenetz wrote: > Does this need to be arbitrary? If the code changes, we should change the test. > We can set the history and verify exactly that it is observed in test. Modified the test. Please see my other comment about keeping the history.
https://codereview.chromium.org/1162203009/diff/1/media/base/android/access_u... File media/base/android/access_unit_queue.h (right): https://codereview.chromium.org/1162203009/diff/1/media/base/android/access_u... media/base/android/access_unit_queue.h:73: typedef std::list<scoped_ptr<DemuxerData>> DataChunkQueue; On 2015/06/11 19:21:20, wolenetz wrote: > STL container > internals don't use scoped_ptr.Pass(), TakesOwnership(), etc, when they move > things around in memory. aside: Exactly this makes me believe that invalid usage of scoped_ptr in STL containers or algorithms won't compile. This does not change the fact that I happily went with raw pointer in this CL.
On 2015/06/11 21:32:04, Tima Vaisburd wrote: > https://codereview.chromium.org/1162203009/diff/1/media/base/android/access_u... > File media/base/android/access_unit_queue.h (right): > > https://codereview.chromium.org/1162203009/diff/1/media/base/android/access_u... > media/base/android/access_unit_queue.h:73: typedef > std::list<scoped_ptr<DemuxerData>> DataChunkQueue; > On 2015/06/11 19:21:20, wolenetz wrote: > > STL container > > internals don't use scoped_ptr.Pass(), TakesOwnership(), etc, when they move > > things around in memory. > > aside: Exactly this makes me believe that invalid usage of scoped_ptr in STL > containers or algorithms won't compile. This does not change the fact that I > happily went with raw pointer in this CL. Training took the full time plus a little. I'll continue CR early in the morning tomorrow.
looking close to done :) https://codereview.chromium.org/1162203009/diff/20001/media/base/android/acce... File media/base/android/access_unit_queue.cc (right): https://codereview.chromium.org/1162203009/diff/20001/media/base/android/acce... media/base/android/access_unit_queue.cc:137: current_chunk_ = --rchunk.base(); On 2015/06/11 20:54:08, Tima Vaisburd wrote: > On 2015/06/11 19:21:20, wolenetz wrote: > > would be good to test this. > > I added a new test KeyFrameWithLongHistory just because of this code. It should > cover. Acknowledged. https://codereview.chromium.org/1162203009/diff/80001/media/base/android/acce... File media/base/android/access_unit_queue.cc (right): https://codereview.chromium.org/1162203009/diff/80001/media/base/android/acce... media/base/android/access_unit_queue.cc:60: DemuxerData* chunk = new DemuxerData(data); aside: Is it always necessary to copy data? This could constitute some small perf overhead. Maybe separate CL to follow-up if you see a way to eliminate/reduce such copying. https://codereview.chromium.org/1162203009/diff/80001/media/base/android/acce... media/base/android/access_unit_queue.cc:99: nit: remove empty line https://codereview.chromium.org/1162203009/diff/80001/media/base/android/acce... media/base/android/access_unit_queue.cc:106: STLDeleteContainerPointers(chunks_.begin(), first_to_keep); my stl-fu is failing me: does this and the following line cause double-free? (I don't think so, but would be good to be sure...) https://codereview.chromium.org/1162203009/diff/80001/media/base/android/acce... File media/base/android/access_unit_queue.h (right): https://codereview.chromium.org/1162203009/diff/80001/media/base/android/acce... media/base/android/access_unit_queue.h:31: // before the caller calls these methods. nit: Clarify in comment(s) here and/or GetInfo() that the AccessUnitQueue owns the pointers; the caller should not delete |front_unit| or |configs|. https://codereview.chromium.org/1162203009/diff/80001/media/base/android/acce... File media/base/android/access_unit_queue_unittest.cc (right): https://codereview.chromium.org/1162203009/diff/80001/media/base/android/acce... media/base/android/access_unit_queue_unittest.cc:129: nit: drop extra blank line. https://codereview.chromium.org/1162203009/diff/80001/media/base/android/acce... media/base/android/access_unit_queue_unittest.cc:136: EXPECT_GE(1U, au_queue.NumChunksForTesting()); s/GE(1U/EQ(0/ : Each of 100 iterations, we push a chunk and advance through all of its access units, so the last Advance() along with default of 0 history should cause au_queue to become empty, right? https://codereview.chromium.org/1162203009/diff/80001/media/base/android/acce... media/base/android/access_unit_queue_unittest.cc:148: EXPECT_LE(5U, au_queue.NumChunksForTesting()); Once we've advanced past the end of the 5th chunk (i==4 here), there should be exactly 5 chunks at this point. s/LE(5U/EQ(0/ here and put into clause where if (i >= 4) this is checked. https://codereview.chromium.org/1162203009/diff/80001/media/base/android/acce... media/base/android/access_unit_queue_unittest.cc:150: EXPECT_GE(6U, au_queue.NumChunksForTesting()); We know exactly how many chunks should be there: put expectation in else clause (so i < 4, then EXPECT_EQ(i+1, ...))
https://codereview.chromium.org/1162203009/diff/80001/media/base/android/acce... File media/base/android/access_unit_queue.cc (right): https://codereview.chromium.org/1162203009/diff/80001/media/base/android/acce... media/base/android/access_unit_queue.cc:106: STLDeleteContainerPointers(chunks_.begin(), first_to_keep); On 2015/06/12 22:05:19, wolenetz wrote: > my stl-fu is failing me: does this and the following line cause double-free? (I > don't think so, but would be good to be sure...) No double-free. Ignore my fu failure :)
https://codereview.chromium.org/1162203009/diff/80001/media/base/android/acce... File media/base/android/access_unit_queue.cc (right): https://codereview.chromium.org/1162203009/diff/80001/media/base/android/acce... media/base/android/access_unit_queue.cc:60: DemuxerData* chunk = new DemuxerData(data); On 2015/06/12 22:05:19, wolenetz wrote: > aside: Is it always necessary to copy data? This could constitute some small > perf overhead. Maybe separate CL to follow-up if you see a way to > eliminate/reduce such copying. Acknowledged. https://codereview.chromium.org/1162203009/diff/80001/media/base/android/acce... media/base/android/access_unit_queue.cc:99: On 2015/06/12 22:05:19, wolenetz wrote: > nit: remove empty line Done. https://codereview.chromium.org/1162203009/diff/80001/media/base/android/acce... File media/base/android/access_unit_queue.h (right): https://codereview.chromium.org/1162203009/diff/80001/media/base/android/acce... media/base/android/access_unit_queue.h:31: // before the caller calls these methods. On 2015/06/12 22:05:19, wolenetz wrote: > nit: Clarify in comment(s) here and/or GetInfo() that the AccessUnitQueue owns > the pointers; the caller should not delete |front_unit| or |configs|. Done. https://codereview.chromium.org/1162203009/diff/80001/media/base/android/acce... File media/base/android/access_unit_queue_unittest.cc (right): https://codereview.chromium.org/1162203009/diff/80001/media/base/android/acce... media/base/android/access_unit_queue_unittest.cc:129: On 2015/06/12 22:05:19, wolenetz wrote: > nit: drop extra blank line. Done. https://codereview.chromium.org/1162203009/diff/80001/media/base/android/acce... media/base/android/access_unit_queue_unittest.cc:136: EXPECT_GE(1U, au_queue.NumChunksForTesting()); On 2015/06/12 22:05:19, wolenetz wrote: > s/GE(1U/EQ(0/ : Each of 100 iterations, we push a chunk and advance through all > of its access units, so the last Advance() along with default of 0 history > should cause au_queue to become empty, right? Yes, my bad thinking. I thought for some reason it's either 0 or 1, probably thinking that the check is done inside the inner loop. Fixed here and below. https://codereview.chromium.org/1162203009/diff/80001/media/base/android/acce... media/base/android/access_unit_queue_unittest.cc:148: EXPECT_LE(5U, au_queue.NumChunksForTesting()); On 2015/06/12 22:05:19, wolenetz wrote: > Once we've advanced past the end of the 5th chunk (i==4 here), there should be > exactly 5 chunks at this point. s/LE(5U/EQ(0/ here and put into clause where if > (i >= 4) this is checked. Done. https://codereview.chromium.org/1162203009/diff/80001/media/base/android/acce... media/base/android/access_unit_queue_unittest.cc:150: EXPECT_GE(6U, au_queue.NumChunksForTesting()); On 2015/06/12 22:05:19, wolenetz wrote: > We know exactly how many chunks should be there: put expectation in else clause > (so i < 4, then EXPECT_EQ(i+1, ...)) Done.
lgtm % nit \o/ https://codereview.chromium.org/1162203009/diff/100001/media/base/android/acc... File media/base/android/access_unit_queue_unittest.cc (right): https://codereview.chromium.org/1162203009/diff/100001/media/base/android/acc... media/base/android/access_unit_queue_unittest.cc:147: EXPECT_EQ(i+1, au_queue.NumChunksForTesting()); nit: s/i+1/i + 1/
The CQ bit was checked by timav@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from qinmin@chromium.org, wolenetz@chromium.org Link to the patchset: https://codereview.chromium.org/1162203009/#ps120001 (title: "Fixed one formatting issue.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1162203009/120001
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/3edc8c797947611b45dc30a9d5a34a6d467a43e6 Cr-Commit-Position: refs/heads/master@{#334294} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
