Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2090343004:
Add GetUrl to DemuxerStreamProvider interface (Closed)
|
Created:
4 years, 6 months ago by tguilbert Modified:
4 years, 6 months ago CC:
Aaron Boodman, abarth-chromium, alokp+watch_chromium.org, avayvod+watch_chromium.org, ben+mojo_chromium.org, chromium-reviews, darin (slow to review), feature-media-reviews_chromium.org, mlamouri+watch-media_chromium.org, posciak+watch_chromium.org, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, xhwang, yzshen+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd GetUrl to DemuxerStreamProvider interface
The Android MediaPlayer manages its own network calls and its own
demuxing, and therefore has no need for a Demuxer or DemuxerStreams.
However, the concept of a Demuxer and DemuxerStream is deeply engrained
within the Pipeline. Removing the Demuxer from the pipeline when using
a MediaPlayer would require a non-trivial amount of refactoring, that
might not be worth it.
This change adds a GetUrl method to the DemuxerStreamProvider interface.
It allows us to pass a Url to the MojoRenderer and MediaPlayerRenderer
at initialization time. It also adds the MediaUrlProvider, a concrete
class implementing this new method. MediaUrlProvider's primary purpose
is however to act as a dummy demuxer, that swallows demuxer calls as to
not disrupt the normal Pipeline logic.
As of this change, the DemuxerStreamProvider interface is now a bit of a
misnomer, but renaming it "MediaSourceProvider" would be even more
confusing. The interface should be renamed if ever we refactor Pipeline
to be less intertwined with Demuxer.
BUG=622812
TEST=All media_unittests passed.
Committed: https://crrev.com/ecbf9a03be35bb77f01fb681e08b822db4575b7e
Cr-Commit-Position: refs/heads/master@{#402048}
Patch Set 1 #Patch Set 2 #Patch Set 3 : added UTs #
Total comments: 22
Patch Set 4 : addressing comments #
Total comments: 22
Patch Set 5 : cleaning up comment text #Patch Set 6 #
Total comments: 14
Patch Set 7 #
Total comments: 8
Patch Set 8 #
Total comments: 1
Messages
Total messages: 23 (8 generated)
tguilbert@chromium.org changed reviewers: + dalecurtis@google.com
PTAL :) Thank you! Thomas
dalecurtis@chromium.org changed reviewers: + dalecurtis@chromium.org - dalecurtis@google.com
dalecurtis@chromium.org changed reviewers: + xhwang@chromium.org
+xhwang for his thoughts before he heads out :) https://codereview.chromium.org/2090343004/diff/40001/media/base/android/medi... File media/base/android/media_url_provider.cc (right): https://codereview.chromium.org/2090343004/diff/40001/media/base/android/medi... media/base/android/media_url_provider.cc:19: GURL* MediaUrlProvider::GetUrl() { Don't typically return ptrs like this. I think for GURL it's comment to return a copy or if this class will outlive all callers a const& is fine. https://codereview.chromium.org/2090343004/diff/40001/media/base/android/medi... media/base/android/media_url_provider.cc:35: DVLOG(1) << __FUNCTION__; DVLOG(2) at least for this type of log, or delete all together if useless. https://codereview.chromium.org/2090343004/diff/40001/media/base/android/medi... media/base/android/media_url_provider.cc:54: // TODO(tguilbert): Investigate if we need to fetch information from the I don't think this works today if it's > 0. Dunno if HLS clips can even start at t > 0. I guess they should be able to, but don't know how we represent that to blink. https://codereview.chromium.org/2090343004/diff/40001/media/base/android/medi... media/base/android/media_url_provider.cc:58: base::Time MediaUrlProvider::GetTimelineOffset() const { I believe this should always be zero. https://codereview.chromium.org/2090343004/diff/40001/media/base/android/medi... media/base/android/media_url_provider.cc:65: // TODO(tguilbert): Verify this is an acceptable return value. It is acceptable :) https://codereview.chromium.org/2090343004/diff/40001/media/base/android/medi... File media/base/android/media_url_provider.h (right): https://codereview.chromium.org/2090343004/diff/40001/media/base/android/medi... media/base/android/media_url_provider.h:5: #ifndef MEDIA_BASE_ANDROID_MEDIA_URL_PRODIVDER_H_ watk@ is going to have a fit, PRODIVDER? :p https://codereview.chromium.org/2090343004/diff/40001/media/base/android/medi... media/base/android/media_url_provider.h:29: class MediaUrlProvider : public Demuxer { It's called a provider, but extends a Demuxer? :) I think despite it's purpose you might want to call it a MediaUrlDemuxer. https://codereview.chromium.org/2090343004/diff/40001/media/base/android/medi... media/base/android/media_url_provider.h:31: MediaUrlProvider(const GURL& url); Use explicit with single parameter constructors. https://codereview.chromium.org/2090343004/diff/40001/media/base/demuxer_stre... File media/base/demuxer_stream_provider.h (right): https://codereview.chromium.org/2090343004/diff/40001/media/base/demuxer_stre... media/base/demuxer_stream_provider.h:32: Should we have a type() or something that says which method should be used? https://codereview.chromium.org/2090343004/diff/40001/media/base/demuxer_stre... media/base/demuxer_stream_provider.h:36: virtual DemuxerStream* GetStream(DemuxerStream::Type type) = 0; Did you consider implementing DemuxerStream instead? I.e. having a DemuxerStream::Type of URL? https://codereview.chromium.org/2090343004/diff/40001/media/base/demuxer_stre... media/base/demuxer_stream_provider.h:41: virtual GURL* GetUrl() = 0; I think this is gnarly enough that we should make this an abstract method to avoid cluttering other implementors.
https://codereview.chromium.org/2090343004/diff/40001/media/base/android/medi... File media/base/android/media_url_provider.cc (right): https://codereview.chromium.org/2090343004/diff/40001/media/base/android/medi... media/base/android/media_url_provider.cc:19: GURL* MediaUrlProvider::GetUrl() { On 2016/06/24 00:42:30, DaleCurtis wrote: > Don't typically return ptrs like this. I think for GURL it's comment to return a > copy or if this class will outlive all callers a const& is fine. Yeah... I knew this would come up, and if it didn't I would have double checked in person. My problem arose from wanting to distinguish between the empty case and the null case. I think the null case should probably not be a concern if I were to add the DemuxerStreamProvider::Type as mentioned in the other file. https://codereview.chromium.org/2090343004/diff/40001/media/base/android/medi... media/base/android/media_url_provider.cc:35: DVLOG(1) << __FUNCTION__; On 2016/06/24 00:42:30, DaleCurtis wrote: > DVLOG(2) at least for this type of log, or delete all together if useless. Done. https://codereview.chromium.org/2090343004/diff/40001/media/base/android/medi... media/base/android/media_url_provider.cc:54: // TODO(tguilbert): Investigate if we need to fetch information from the On 2016/06/24 00:42:29, DaleCurtis wrote: > I don't think this works today if it's > 0. Dunno if HLS clips can even start at > t > 0. I guess they should be able to, but don't know how we represent that to > blink. My questioning came from the fact that, in the current prototype, if you toggle full-screen (off/on/off), the timestamp is not preserved. The start time is probably being picked up from somewhere else, but I just left this as a reminder of a possible place to investigate. https://codereview.chromium.org/2090343004/diff/40001/media/base/android/medi... media/base/android/media_url_provider.cc:58: base::Time MediaUrlProvider::GetTimelineOffset() const { On 2016/06/24 00:42:29, DaleCurtis wrote: > I believe this should always be zero. Acknowledged. https://codereview.chromium.org/2090343004/diff/40001/media/base/android/medi... media/base/android/media_url_provider.cc:65: // TODO(tguilbert): Verify this is an acceptable return value. On 2016/06/24 00:42:30, DaleCurtis wrote: > It is acceptable :) Thank you! https://codereview.chromium.org/2090343004/diff/40001/media/base/android/medi... File media/base/android/media_url_provider.h (right): https://codereview.chromium.org/2090343004/diff/40001/media/base/android/medi... media/base/android/media_url_provider.h:5: #ifndef MEDIA_BASE_ANDROID_MEDIA_URL_PRODIVDER_H_ On 2016/06/24 00:42:30, DaleCurtis wrote: > watk@ is going to have a fit, PRODIVDER? :p Woops! :P https://codereview.chromium.org/2090343004/diff/40001/media/base/android/medi... media/base/android/media_url_provider.h:29: class MediaUrlProvider : public Demuxer { On 2016/06/24 00:42:30, DaleCurtis wrote: > It's called a provider, but extends a Demuxer? :) I think despite it's purpose > you might want to call it a MediaUrlDemuxer. Done. https://codereview.chromium.org/2090343004/diff/40001/media/base/android/medi... media/base/android/media_url_provider.h:31: MediaUrlProvider(const GURL& url); On 2016/06/24 00:42:30, DaleCurtis wrote: > Use explicit with single parameter constructors. Done. https://codereview.chromium.org/2090343004/diff/40001/media/base/demuxer_stre... File media/base/demuxer_stream_provider.h (right): https://codereview.chromium.org/2090343004/diff/40001/media/base/demuxer_stre... media/base/demuxer_stream_provider.h:32: On 2016/06/24 00:42:30, DaleCurtis wrote: > Should we have a type() or something that says which method should be used? This sounds good to me. However, I have been flip-flopping between making this an abstract method or not. While I prefer forcing subclasses to implement their own type(), since GetUrl() is abstract, it's simpler and less clutter to make this abstract. WDYT? I am open to either solutions. https://codereview.chromium.org/2090343004/diff/40001/media/base/demuxer_stre... media/base/demuxer_stream_provider.h:36: virtual DemuxerStream* GetStream(DemuxerStream::Type type) = 0; On 2016/06/24 00:42:30, DaleCurtis wrote: > Did you consider implementing DemuxerStream instead? I.e. having a > DemuxerStream::Type of URL? I did, and that is how I originally implemented it in my MediaPlayerRenderer prototype CL. I had the DemuxerStream::Type::URL CL ready for review, and then, after thinking about xhwang@'s comment on my prototype and talking to him offline, I decided this way was probably cleaner. Because the CL was ready to go however, I uploaded it earlier today, closed it and added it to the bug attached to CL. Here is the link again for reference: https://codereview.chromium.org/2099453002 The current CL also addresses the question I had trouble phrasing when we went over my prototype CL in person ("Is the presence of a DemuxerStream of type URL a sufficiently clear indicator to let WMPI/readers know that we are operating in a 'special case', or should a more explicit flag be set?"). Using GetUrl() and the DemuxerStreamProvider::Type::URL is clearer than the DemuxerStream of type URL. In one case, different code paths are taken based off the *nature* of the Demuxer; in the other, different paths are taken based of the presence/absence of certain stream types, which tells us implicitly tell us something about the nature of the Demuxer (but is not obvious for a first time reader). The above paragraph might have been a bit dense... I will gladly discuss this offline if need be! https://codereview.chromium.org/2090343004/diff/40001/media/base/demuxer_stre... media/base/demuxer_stream_provider.h:41: virtual GURL* GetUrl() = 0; On 2016/06/24 00:42:30, DaleCurtis wrote: > I think this is gnarly enough that we should make this an abstract method to > avoid cluttering other implementors. I am fine with this if you are.
Looking pretty good. Mostly nits. https://chromiumcodereview.appspot.com/2090343004/diff/60001/media/base/andro... File media/base/android/media_url_demuxer.cc (right): https://chromiumcodereview.appspot.com/2090343004/diff/60001/media/base/andro... media/base/android/media_url_demuxer.cc:16: return nullptr; NOTREACHED() https://chromiumcodereview.appspot.com/2090343004/diff/60001/media/base/andro... media/base/android/media_url_demuxer.cc:27: void MediaUrlDemuxer::Initialize(DemuxerHost* host, This function and below should be NOTREACHED()? https://chromiumcodereview.appspot.com/2090343004/diff/60001/media/base/andro... File media/base/android/media_url_demuxer.h (right): https://chromiumcodereview.appspot.com/2090343004/diff/60001/media/base/andro... media/base/android/media_url_demuxer.h:37: Type type() final { return DemuxerStreamProvider::Type::URL; }; nit: We don't really distinguish between override and final. Now you use them both and people may wonder why. I don't think we expect people to extend this class and override any class again. Maybe use final for the class, or mark all methods final or override, just to avoid confusion. https://chromiumcodereview.appspot.com/2090343004/diff/60001/media/base/andro... File media/base/android/media_url_demuxer_unittest.cc (right): https://chromiumcodereview.appspot.com/2090343004/diff/60001/media/base/andro... media/base/android/media_url_demuxer_unittest.cc:30: base::MessageLoop message_loop_; Use RunLoop now. Actually, why do you need a messageloop for this test? https://chromiumcodereview.appspot.com/2090343004/diff/60001/media/base/andro... media/base/android/media_url_demuxer_unittest.cc:35: TEST_F(MediaUrlDemuxerTest, DoesNotReturnStreams) { Check the type. https://chromiumcodereview.appspot.com/2090343004/diff/60001/media/base/andro... media/base/android/media_url_demuxer_unittest.cc:40: DemuxerStream* text_stream = demuxer_->GetStream(DemuxerStream::Type::TEXT); We can specify in the API that if the type is URL, then GetStream() should not be called. Then we don't need to test this. https://chromiumcodereview.appspot.com/2090343004/diff/60001/media/base/demux... File media/base/demuxer_stream_provider.h (right): https://chromiumcodereview.appspot.com/2090343004/diff/60001/media/base/demux... media/base/demuxer_stream_provider.h:40: // Returns a null value if we are of Type::URL. You can say that if type is not STREAM, this should not be called. Then we can just DCHECK/NOTREACHED in impl. https://chromiumcodereview.appspot.com/2090343004/diff/60001/media/base/demux... media/base/demuxer_stream_provider.h:45: // Returns an empty GURL if we are of Type::STREAM. ditto https://chromiumcodereview.appspot.com/2090343004/diff/60001/media/base/demux... media/base/demuxer_stream_provider.h:46: virtual GURL GetUrl() { return GURL(); }; remote the trailing semicolon, ditto on l.50 https://chromiumcodereview.appspot.com/2090343004/diff/60001/media/base/demux... media/base/demuxer_stream_provider.h:50: virtual DemuxerStreamProvider::Type type() { return Type::STREAM; }; nit: Will this work? Type type() { return STREAM; }
https://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Stop-i... https://chromiumcodereview.appspot.com/2090343004/diff/60001/media/base/demux... File media/base/demuxer_stream_provider.h (right): https://chromiumcodereview.appspot.com/2090343004/diff/60001/media/base/demux... media/base/demuxer_stream_provider.h:46: virtual GURL GetUrl() { return GURL(); }; On 2016/06/24 at 03:19:47, xhwang wrote: > remote the trailing semicolon, ditto on l.50 Virtual methods should never be inline per the style guide.
https://chromiumcodereview.appspot.com/2090343004/diff/60001/media/base/andro... File media/base/android/media_url_demuxer.cc (right): https://chromiumcodereview.appspot.com/2090343004/diff/60001/media/base/andro... media/base/android/media_url_demuxer.cc:16: return nullptr; On 2016/06/24 03:19:47, xhwang wrote: > NOTREACHED() Done. https://chromiumcodereview.appspot.com/2090343004/diff/60001/media/base/andro... media/base/android/media_url_demuxer.cc:27: void MediaUrlDemuxer::Initialize(DemuxerHost* host, On 2016/06/24 03:19:47, xhwang wrote: > This function and below should be NOTREACHED()? They should not. A lot of them are indeed called by Pipeline, and it is easier for the moment to NOP them then to refactor Pipeline. https://chromiumcodereview.appspot.com/2090343004/diff/60001/media/base/andro... File media/base/android/media_url_demuxer.h (right): https://chromiumcodereview.appspot.com/2090343004/diff/60001/media/base/andro... media/base/android/media_url_demuxer.h:37: Type type() final { return DemuxerStreamProvider::Type::URL; }; On 2016/06/24 03:19:47, xhwang wrote: > nit: We don't really distinguish between override and final. Now you use them > both and people may wonder why. I don't think we expect people to extend this > class and override any class again. Maybe use final for the class, or mark all > methods final or override, just to avoid confusion. Done. https://chromiumcodereview.appspot.com/2090343004/diff/60001/media/base/andro... File media/base/android/media_url_demuxer_unittest.cc (right): https://chromiumcodereview.appspot.com/2090343004/diff/60001/media/base/andro... media/base/android/media_url_demuxer_unittest.cc:30: base::MessageLoop message_loop_; On 2016/06/24 03:19:47, xhwang wrote: > Use RunLoop now. > > Actually, why do you need a messageloop for this test? I don't, removed! (artifact of copy-pasting) https://chromiumcodereview.appspot.com/2090343004/diff/60001/media/base/andro... media/base/android/media_url_demuxer_unittest.cc:35: TEST_F(MediaUrlDemuxerTest, DoesNotReturnStreams) { On 2016/06/24 03:19:47, xhwang wrote: > Check the type. Done. https://chromiumcodereview.appspot.com/2090343004/diff/60001/media/base/andro... media/base/android/media_url_demuxer_unittest.cc:40: DemuxerStream* text_stream = demuxer_->GetStream(DemuxerStream::Type::TEXT); On 2016/06/24 03:19:47, xhwang wrote: > We can specify in the API that if the type is URL, then GetStream() should not > be called. Then we don't need to test this. Done. https://chromiumcodereview.appspot.com/2090343004/diff/60001/media/base/demux... File media/base/demuxer_stream_provider.h (right): https://chromiumcodereview.appspot.com/2090343004/diff/60001/media/base/demux... media/base/demuxer_stream_provider.h:40: // Returns a null value if we are of Type::URL. On 2016/06/24 03:19:47, xhwang wrote: > You can say that if type is not STREAM, this should not be called. Then we can > just DCHECK/NOTREACHED in impl. Done. https://chromiumcodereview.appspot.com/2090343004/diff/60001/media/base/demux... media/base/demuxer_stream_provider.h:45: // Returns an empty GURL if we are of Type::STREAM. On 2016/06/24 03:19:47, xhwang wrote: > ditto Done. https://chromiumcodereview.appspot.com/2090343004/diff/60001/media/base/demux... media/base/demuxer_stream_provider.h:46: virtual GURL GetUrl() { return GURL(); }; On 2016/06/24 03:19:47, xhwang wrote: > remote the trailing semicolon, ditto on l.50 Done. https://chromiumcodereview.appspot.com/2090343004/diff/60001/media/base/demux... media/base/demuxer_stream_provider.h:46: virtual GURL GetUrl() { return GURL(); }; On 2016/06/24 17:43:41, DaleCurtis wrote: > On 2016/06/24 at 03:19:47, xhwang wrote: > > remote the trailing semicolon, ditto on l.50 > > Virtual methods should never be inline per the style guide. Done. https://chromiumcodereview.appspot.com/2090343004/diff/60001/media/base/demux... media/base/demuxer_stream_provider.h:50: virtual DemuxerStreamProvider::Type type() { return Type::STREAM; }; On 2016/06/24 03:19:47, xhwang wrote: > nit: > > Will this work? > > Type type() { return STREAM; } It does.
Description was changed from ========== Add GetUrl to DemuxerStreamProvider interface The Android MediaPlayer manages its own network calls and its own demuxing, and therefore has no need for a Demuxer or DemuxerStreams. However, the concept of a Demuxer and DemuxerStream is deeply engrained within the Pipeline. Removing the Demuxer from the pipeline when using a MediaPlayer would require a non-trivial amount of refactoring, that might not be worth it. This change adds a GetUrl method to the DemuxerStreamProvider interface. It allows us to pass a Url to the MojoRenderer and MediaPlayerRenderer at initialization time. It also adds the MediaUrlProvider, a concrete class implementing this new method. MediaUrlProvider's primary purpose is however to act as a dummy demuxer, that swallows demuxer calls as to not disrupt the normal Pipeline logic. As of this change, the DemuxerStreamProvider interface is now a bit of a misnomer, but renaming it "MediaSourceProvider" would be even more confusing. The interface should be renamed if ever we refactor Pipeline to be less intertwined with Demuxer. BUG=622812 ========== to ========== Add GetUrl to DemuxerStreamProvider interface The Android MediaPlayer manages its own network calls and its own demuxing, and therefore has no need for a Demuxer or DemuxerStreams. However, the concept of a Demuxer and DemuxerStream is deeply engrained within the Pipeline. Removing the Demuxer from the pipeline when using a MediaPlayer would require a non-trivial amount of refactoring, that might not be worth it. This change adds a GetUrl method to the DemuxerStreamProvider interface. It allows us to pass a Url to the MojoRenderer and MediaPlayerRenderer at initialization time. It also adds the MediaUrlProvider, a concrete class implementing this new method. MediaUrlProvider's primary purpose is however to act as a dummy demuxer, that swallows demuxer calls as to not disrupt the normal Pipeline logic. As of this change, the DemuxerStreamProvider interface is now a bit of a misnomer, but renaming it "MediaSourceProvider" would be even more confusing. The interface should be renamed if ever we refactor Pipeline to be less intertwined with Demuxer. BUG=622812 TEST=All media_unittests passed. ==========
https://chromiumcodereview.appspot.com/2090343004/diff/100001/media/base/andr... File media/base/android/media_url_demuxer.cc (right): https://chromiumcodereview.appspot.com/2090343004/diff/100001/media/base/andr... media/base/android/media_url_demuxer.cc:7: #include "base/memory/ptr_util.h" What is this for? https://chromiumcodereview.appspot.com/2090343004/diff/100001/media/base/andr... File media/base/android/media_url_demuxer.h (right): https://chromiumcodereview.appspot.com/2090343004/diff/100001/media/base/andr... media/base/android/media_url_demuxer.h:37: Type type() override; Can't be hacker_style() either if not inline. https://chromiumcodereview.appspot.com/2090343004/diff/100001/media/base/andr... File media/base/android/media_url_demuxer_unittest.cc (right): https://chromiumcodereview.appspot.com/2090343004/diff/100001/media/base/andr... media/base/android/media_url_demuxer_unittest.cc:35: ASSERT_EQ(DemuxerStreamProvider::Type::URL, demuxer_->type()); Prefer expect_eq unless the test truly can't proceed any further if this is false. https://chromiumcodereview.appspot.com/2090343004/diff/100001/media/base/andr... media/base/android/media_url_demuxer_unittest.cc:48: Condense to EXPECT_EQ(GURL::EmptyGURL(), demuxer_->GetUrl()); Will be easier to understand when an error occurs since it'll directly print the source of "url". Ditto for above if you want. https://chromiumcodereview.appspot.com/2090343004/diff/100001/media/base/andr... media/base/android/media_url_demuxer_unittest.cc:54: TEST_F(MediaUrlDemuxerTest, VerifyIdempotenceOfGetUrl) { I don't think this test does anything? :) https://chromiumcodereview.appspot.com/2090343004/diff/100001/media/base/demu... File media/base/demuxer_stream_provider.h (right): https://chromiumcodereview.appspot.com/2090343004/diff/100001/media/base/demu... media/base/demuxer_stream_provider.h:49: virtual GURL GetUrl(); const ? https://chromiumcodereview.appspot.com/2090343004/diff/100001/media/base/demu... media/base/demuxer_stream_provider.h:51: virtual DemuxerStreamProvider::Type type(); const?
https://chromiumcodereview.appspot.com/2090343004/diff/100001/media/base/andr... File media/base/android/media_url_demuxer.cc (right): https://chromiumcodereview.appspot.com/2090343004/diff/100001/media/base/andr... media/base/android/media_url_demuxer.cc:7: #include "base/memory/ptr_util.h" On 2016/06/24 22:02:40, DaleCurtis wrote: > What is this for? Removed. https://chromiumcodereview.appspot.com/2090343004/diff/100001/media/base/andr... File media/base/android/media_url_demuxer.h (right): https://chromiumcodereview.appspot.com/2090343004/diff/100001/media/base/andr... media/base/android/media_url_demuxer.h:37: Type type() override; On 2016/06/24 22:02:40, DaleCurtis wrote: > Can't be hacker_style() either if not inline. TIL hacker_style <=> inlining. Done. https://chromiumcodereview.appspot.com/2090343004/diff/100001/media/base/andr... File media/base/android/media_url_demuxer_unittest.cc (right): https://chromiumcodereview.appspot.com/2090343004/diff/100001/media/base/andr... media/base/android/media_url_demuxer_unittest.cc:35: ASSERT_EQ(DemuxerStreamProvider::Type::URL, demuxer_->type()); On 2016/06/24 22:02:40, DaleCurtis wrote: > Prefer expect_eq unless the test truly can't proceed any further if this is > false. Done. https://chromiumcodereview.appspot.com/2090343004/diff/100001/media/base/andr... media/base/android/media_url_demuxer_unittest.cc:48: On 2016/06/24 22:02:40, DaleCurtis wrote: > Condense to EXPECT_EQ(GURL::EmptyGURL(), demuxer_->GetUrl()); Will be easier to > understand when an error occurs since it'll directly print the source of "url". > Ditto for above if you want. Done. https://chromiumcodereview.appspot.com/2090343004/diff/100001/media/base/andr... media/base/android/media_url_demuxer_unittest.cc:54: TEST_F(MediaUrlDemuxerTest, VerifyIdempotenceOfGetUrl) { On 2016/06/24 22:02:40, DaleCurtis wrote: > I don't think this test does anything? :) You are right, I didn't update this :) (When it was GURL*, I wanted to "prove" it was the same pointer and not a new one each time). https://chromiumcodereview.appspot.com/2090343004/diff/100001/media/base/demu... File media/base/demuxer_stream_provider.h (right): https://chromiumcodereview.appspot.com/2090343004/diff/100001/media/base/demu... media/base/demuxer_stream_provider.h:49: virtual GURL GetUrl(); On 2016/06/24 22:02:40, DaleCurtis wrote: > const ? Done. https://chromiumcodereview.appspot.com/2090343004/diff/100001/media/base/demu... media/base/demuxer_stream_provider.h:51: virtual DemuxerStreamProvider::Type type(); On 2016/06/24 22:02:40, DaleCurtis wrote: > const? Done.
lgtm nice stuff! https://chromiumcodereview.appspot.com/2090343004/diff/120001/media/base/andr... File media/base/android/media_url_demuxer.cc (right): https://chromiumcodereview.appspot.com/2090343004/diff/120001/media/base/andr... media/base/android/media_url_demuxer.cc:35: status_cb.Run(PIPELINE_OK); I forget if this is okay to run inline or if it must be posted. https://chromiumcodereview.appspot.com/2090343004/diff/120001/media/base/andr... media/base/android/media_url_demuxer.cc:44: status_cb.Run(PIPELINE_OK); Ditto. https://chromiumcodereview.appspot.com/2090343004/diff/120001/media/base/andr... media/base/android/media_url_demuxer.cc:55: // This seems to be an appropriate value for the moment. Not very useful :) https://chromiumcodereview.appspot.com/2090343004/diff/120001/media/base/andr... File media/base/android/media_url_demuxer_unittest.cc (right): https://chromiumcodereview.appspot.com/2090343004/diff/120001/media/base/andr... media/base/android/media_url_demuxer_unittest.cc:23: const std::string kDefaultUrl = "http://example.com/"; Could just make this a GURL and avoid the GURL(...) above and .spec() below.
https://chromiumcodereview.appspot.com/2090343004/diff/120001/media/base/andr... File media/base/android/media_url_demuxer.cc (right): https://chromiumcodereview.appspot.com/2090343004/diff/120001/media/base/andr... media/base/android/media_url_demuxer.cc:35: status_cb.Run(PIPELINE_OK); On 2016/06/24 23:29:11, DaleCurtis wrote: > I forget if this is okay to run inline or if it must be posted. Done. https://chromiumcodereview.appspot.com/2090343004/diff/120001/media/base/andr... media/base/android/media_url_demuxer.cc:44: status_cb.Run(PIPELINE_OK); On 2016/06/24 23:29:11, DaleCurtis wrote: > Ditto. Done. https://chromiumcodereview.appspot.com/2090343004/diff/120001/media/base/andr... media/base/android/media_url_demuxer.cc:55: // This seems to be an appropriate value for the moment. On 2016/06/24 23:29:11, DaleCurtis wrote: > Not very useful :) Done. https://chromiumcodereview.appspot.com/2090343004/diff/120001/media/base/andr... File media/base/android/media_url_demuxer_unittest.cc (right): https://chromiumcodereview.appspot.com/2090343004/diff/120001/media/base/andr... media/base/android/media_url_demuxer_unittest.cc:23: const std::string kDefaultUrl = "http://example.com/"; On 2016/06/24 23:29:11, DaleCurtis wrote: > Could just make this a GURL and avoid the GURL(...) above and .spec() below. Somewhat changed the UTs in this last patch. https://chromiumcodereview.appspot.com/2090343004/diff/140001/media/base/andr... File media/base/android/media_url_demuxer_unittest.cc (right): https://chromiumcodereview.appspot.com/2090343004/diff/140001/media/base/andr... media/base/android/media_url_demuxer_unittest.cc:35: base::MessageLoop message_loop_; @xhwang: I am aware MessageLoop is being deprecated, but without it, the test does not work. Let me know if you still want me to change it. I have not found an alternative, but am open to suggestions!
Nice work! LGTM!
The CQ bit was checked by tguilbert@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dalecurtis@chromium.org Link to the patchset: https://chromiumcodereview.appspot.com/2090343004/#ps140001 (title: "")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Add GetUrl to DemuxerStreamProvider interface The Android MediaPlayer manages its own network calls and its own demuxing, and therefore has no need for a Demuxer or DemuxerStreams. However, the concept of a Demuxer and DemuxerStream is deeply engrained within the Pipeline. Removing the Demuxer from the pipeline when using a MediaPlayer would require a non-trivial amount of refactoring, that might not be worth it. This change adds a GetUrl method to the DemuxerStreamProvider interface. It allows us to pass a Url to the MojoRenderer and MediaPlayerRenderer at initialization time. It also adds the MediaUrlProvider, a concrete class implementing this new method. MediaUrlProvider's primary purpose is however to act as a dummy demuxer, that swallows demuxer calls as to not disrupt the normal Pipeline logic. As of this change, the DemuxerStreamProvider interface is now a bit of a misnomer, but renaming it "MediaSourceProvider" would be even more confusing. The interface should be renamed if ever we refactor Pipeline to be less intertwined with Demuxer. BUG=622812 TEST=All media_unittests passed. ========== to ========== Add GetUrl to DemuxerStreamProvider interface The Android MediaPlayer manages its own network calls and its own demuxing, and therefore has no need for a Demuxer or DemuxerStreams. However, the concept of a Demuxer and DemuxerStream is deeply engrained within the Pipeline. Removing the Demuxer from the pipeline when using a MediaPlayer would require a non-trivial amount of refactoring, that might not be worth it. This change adds a GetUrl method to the DemuxerStreamProvider interface. It allows us to pass a Url to the MojoRenderer and MediaPlayerRenderer at initialization time. It also adds the MediaUrlProvider, a concrete class implementing this new method. MediaUrlProvider's primary purpose is however to act as a dummy demuxer, that swallows demuxer calls as to not disrupt the normal Pipeline logic. As of this change, the DemuxerStreamProvider interface is now a bit of a misnomer, but renaming it "MediaSourceProvider" would be even more confusing. The interface should be renamed if ever we refactor Pipeline to be less intertwined with Demuxer. BUG=622812 TEST=All media_unittests passed. ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Add GetUrl to DemuxerStreamProvider interface The Android MediaPlayer manages its own network calls and its own demuxing, and therefore has no need for a Demuxer or DemuxerStreams. However, the concept of a Demuxer and DemuxerStream is deeply engrained within the Pipeline. Removing the Demuxer from the pipeline when using a MediaPlayer would require a non-trivial amount of refactoring, that might not be worth it. This change adds a GetUrl method to the DemuxerStreamProvider interface. It allows us to pass a Url to the MojoRenderer and MediaPlayerRenderer at initialization time. It also adds the MediaUrlProvider, a concrete class implementing this new method. MediaUrlProvider's primary purpose is however to act as a dummy demuxer, that swallows demuxer calls as to not disrupt the normal Pipeline logic. As of this change, the DemuxerStreamProvider interface is now a bit of a misnomer, but renaming it "MediaSourceProvider" would be even more confusing. The interface should be renamed if ever we refactor Pipeline to be less intertwined with Demuxer. BUG=622812 TEST=All media_unittests passed. ========== to ========== Add GetUrl to DemuxerStreamProvider interface The Android MediaPlayer manages its own network calls and its own demuxing, and therefore has no need for a Demuxer or DemuxerStreams. However, the concept of a Demuxer and DemuxerStream is deeply engrained within the Pipeline. Removing the Demuxer from the pipeline when using a MediaPlayer would require a non-trivial amount of refactoring, that might not be worth it. This change adds a GetUrl method to the DemuxerStreamProvider interface. It allows us to pass a Url to the MojoRenderer and MediaPlayerRenderer at initialization time. It also adds the MediaUrlProvider, a concrete class implementing this new method. MediaUrlProvider's primary purpose is however to act as a dummy demuxer, that swallows demuxer calls as to not disrupt the normal Pipeline logic. As of this change, the DemuxerStreamProvider interface is now a bit of a misnomer, but renaming it "MediaSourceProvider" would be even more confusing. The interface should be renamed if ever we refactor Pipeline to be less intertwined with Demuxer. BUG=622812 TEST=All media_unittests passed. Committed: https://crrev.com/ecbf9a03be35bb77f01fb681e08b822db4575b7e Cr-Commit-Position: refs/heads/master@{#402048} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/ecbf9a03be35bb77f01fb681e08b822db4575b7e Cr-Commit-Position: refs/heads/master@{#402048} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
