|
|
Created:
5 years, 10 months ago by servolk Modified:
5 years, 7 months ago Reviewers:
Ryan Sleevi, Randy Smith (Not in Mondays), gunsch, bengr, wolenetz, lcwu1, darin (slow to review), ddorwin, scherkus (not reviewing) CC:
chromium-reviews, mlamouri+watch-content_chromium.org, cbentzel+watch_chromium.org, creis+watch_chromium.org, nasko+codewatch_chromium.org, jam, feature-media-reviews_chromium.org, darin-cc_chromium.org, mkwst+moarreviews-renderer_chromium.org, asanka, erickung1, sandersd (OOO until July 31) Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionExtracted media mime type checks from net/base/ into media/base/
This is an old issue that has been discussed a while ago - net/ directory is probably not the right place for mime type checks.
acolwell@ attempted to do some refactoring 6 months ago:
https://codereview.chromium.org/401523002/
But that CL has never landed for some reason and acolwell@ is gone from Chromium. Unfortunately a lot of things have changed in the files touched by that CL in the past 6 months, so I was unable to do an easy cherry-pick. But I've tried to repeat some of that work. This CL is the first part of mime type refactoring.
It moves media-related mime type checks from net/base/ into media/ but leaves the rest of the stuff in the net/base/ for now.
One tricky thing to note here (same as in the previous CL) is that net::IsSupportedNonImageMimeType used to return true for media types and now it won't after this change. So we please pay special attention to that.
I think ultimately we might want to move mime type checks out of net/ completely (perhaps to content/), but I'd prefer to save that for a separate CL/discussion. Looks like one of the reasons the previous CL didn't land was that Aaron tried to solve this problem in one shot, and so CL just kept growing and growing.
BUG=318217
Patch Set 1 #Patch Set 2 : Removed non_image_map_ from media/base/mime_util.cc #Patch Set 3 : Update old references #Patch Set 4 : Add dependency on media/base for mojo #Patch Set 5 : Add mojo to the media/base visibility list #Patch Set 6 : Make media/blink depend on media/base (due to using IsSupportedMediaMimeType) #Patch Set 7 : Rebased changes onto ToT #Patch Set 8 : Fixed android build #Patch Set 9 : Added #include media/base/mime_util.h into shell_main_delegate.cc #
Total comments: 4
Patch Set 10 : Introduced content::IsSupportedMimeType and deprecated the net/ one #Patch Set 11 : Build fixes #1 #
Total comments: 3
Messages
Total messages: 62 (5 generated)
On 2015/01/29 00:26:37, servolk wrote: At the risk of being a pain, I think we need to force that discussion for //content now, rather than save it. I don't want us to move things out piece-meal - it really is too hard to reason about the correctness and side-effects. Nothing in //net needs to depend on these functions (mod one "seemingly wrong" bit of code), so we should really be trying to lift it up wholesale out.
On 2015/01/29 00:45:29, Ryan Sleevi wrote: > On 2015/01/29 00:26:37, servolk wrote: > > At the risk of being a pain, I think we need to force that discussion for > //content now, rather than save it. > > I don't want us to move things out piece-meal - it really is too hard to reason > about the correctness and side-effects. Nothing in //net needs to depend on > these functions (mod one "seemingly wrong" bit of code), so we should really be > trying to lift it up wholesale out. Ryan, I understand your concern. But I'd like to point out that having a single large CL doesn't make it any easier to reason about correctness and side-effects (since to separate things out properly we'd probably still have to change the behavior of IsSupportedNonImageMimeType to return false for media types), in fact I believe it makes things harder to review. By having separate CLs we can keep the scope of each change somewhat limited. I'm all for moving the rest of the mime_util stuff into content/, but I don't have very good understanding of Chromium architectural layers. From what I gathered in the previous discussion on the old CL, it seems like net/ is a relatively low-level layer and it shouldn't depend on either content/ or media/ directly, so if we want to fix the usage of IsSupportedMimeType in net/filter/filter.cc we probably need to pass a callback to content::IsSupportedMimeType there somehow, right? Looks like there are only 3 subclasses of net::Filter and only net::GzipFilter really needs that code. Do you know who somebody more familiar with this?
On 2015/01/29 01:02:54, servolk wrote: > On 2015/01/29 00:45:29, Ryan Sleevi wrote: > > On 2015/01/29 00:26:37, servolk wrote: > > > > At the risk of being a pain, I think we need to force that discussion for > > //content now, rather than save it. > > > > I don't want us to move things out piece-meal - it really is too hard to > reason > > about the correctness and side-effects. Nothing in //net needs to depend on > > these functions (mod one "seemingly wrong" bit of code), so we should really > be > > trying to lift it up wholesale out. > > Ryan, I understand your concern. But I'd like to point out that having a single > large CL doesn't make it any easier to reason about correctness and side-effects > (since to separate things out properly we'd probably still have to change the > behavior of IsSupportedNonImageMimeType to return false for media types), in > fact I believe it makes things harder to review. By having separate CLs we can > keep the scope of each change somewhat limited. > I'm all for moving the rest of the mime_util stuff into content/, but I don't > have very good understanding of Chromium architectural layers. From what I > gathered in the previous discussion on the old CL, it seems like net/ is a > relatively low-level layer and it shouldn't depend on either content/ or media/ > directly, so if we want to fix the usage of IsSupportedMimeType in > net/filter/filter.cc we probably need to pass a callback to > content::IsSupportedMimeType there somehow, right? Looks like there are only 3 > subclasses of net::Filter and only net::GzipFilter really needs that code. Do > you know who somebody more familiar with this? Correct, //net is beneath both, which means moving out the dependencies. AFAICT, this means: - IsSupportedMimeType in net/filter/filter.cc: ^ I suspect this can be removed entirely. It's... exceptionally weird. rdsmith@ and cbentzel@ are best to have current and historical context - IsSupportedNonImageMimeType in net/url_request/url_request_http_job.cc for IsCompressibleContent(): ^ This is a semi-layering violation done for convenience for histograms for compression. This only applies to SDCH, and is itself extremely old. rdsmith@ has the best current context on this Beyond that, the remaining uses are for parsing mime types and determining filenames (for download). These can likely eventually move out to //content as well, but may need to stay in //net (or move to //components) due to iOS considerations. However, with the exception of the two above, we can wholesale hoist IsSupported* entirely out, it seems, which avoids any weird intermediate steps.
rsleevi@chromium.org changed reviewers: + rdsmith@chromium.org
Randy: Payback time. Would you mind checking my previous comment re: SDCH & filter code and give your thoughts? It seems like we could prune some code and come out ahead.
[+asanka FHI for the download interaction with Filter::FixupEncodingTypes.] On 2015/01/29 01:16:07, Ryan Sleevi wrote: > On 2015/01/29 01:02:54, servolk wrote: > > On 2015/01/29 00:45:29, Ryan Sleevi wrote: > > > On 2015/01/29 00:26:37, servolk wrote: > > > > > > At the risk of being a pain, I think we need to force that discussion for > > > //content now, rather than save it. > > > > > > I don't want us to move things out piece-meal - it really is too hard to > > reason > > > about the correctness and side-effects. Nothing in //net needs to depend on > > > these functions (mod one "seemingly wrong" bit of code), so we should really > > be > > > trying to lift it up wholesale out. > > > > Ryan, I understand your concern. But I'd like to point out that having a > single > > large CL doesn't make it any easier to reason about correctness and > side-effects > > (since to separate things out properly we'd probably still have to change the > > behavior of IsSupportedNonImageMimeType to return false for media types), in > > fact I believe it makes things harder to review. By having separate CLs we can > > keep the scope of each change somewhat limited. > > I'm all for moving the rest of the mime_util stuff into content/, but I don't > > have very good understanding of Chromium architectural layers. From what I > > gathered in the previous discussion on the old CL, it seems like net/ is a > > relatively low-level layer and it shouldn't depend on either content/ or > media/ > > directly, so if we want to fix the usage of IsSupportedMimeType in > > net/filter/filter.cc we probably need to pass a callback to > > content::IsSupportedMimeType there somehow, right? Looks like there are only 3 > > subclasses of net::Filter and only net::GzipFilter really needs that code. Do > > you know who somebody more familiar with this? > > Correct, //net is beneath both, which means moving out the dependencies. > > AFAICT, this means: > - IsSupportedMimeType in net/filter/filter.cc: > ^ I suspect this can be removed entirely. It's... exceptionally weird. > rdsmith@ and cbentzel@ are best to have current and historical context "exceptionally weird". That description seems precise. I'll admit that my brain bends somewhat looking at that code, but I don't think it can be simply removed. It looks like it's the dual of the code in BufferredResourceHandler that decides on the fly whether or not to download requests, and that the combination of the two pieces of code means that when we don't have a supported mime type, Content-Encoding: gzip is active, and the file has a .gz extension, we'll won't uncompress the file on download (so that the user doesn't get a .gz file on their disk that isn't actually gzipped). There's a lot wrong with this, both the ugly layering violation around the interaction with BufferredResourceHandler and the stupidity of the server in saying "Content-Encoding: gzip" for a resource that's gzipped. My inclination is to think that we should check to see whether any servers are actually doing that with some UMA, and if they're not, remove the code. But if there are servers that are doing this and we remove the code, we'll have unhappy users, and shooting from the hip I'm not certain how to solve the layering violation, though there's probably a way to do it. BufferredResourceHandler gets into the act after we've already started handling the URLRequest response. I suppose we could do the non-sniffing aspects of deciding whether something is a download or not by delegate earlier in the process. > - IsSupportedNonImageMimeType in net/url_request/url_request_http_job.cc for > IsCompressibleContent(): > ^ This is a semi-layering violation done for convenience for histograms for > compression. This only applies to SDCH, and is itself extremely old. rdsmith@ > has the best current context on this Hmmm. Why do you say it applies only to SDCH? It looks to me like it applies to gzip as well. Having said that, the histograms have no owners, and I've never looked at them in the SDCH context, so I'd be happy nuking them from orbit. Anyone else we should ask? > Beyond that, the remaining uses are for parsing mime types and determining > filenames (for download). These can likely eventually move out to //content as > well, but may need to stay in //net (or move to //components) due to iOS > considerations. > > However, with the exception of the two above, we can wholesale hoist > IsSupported* entirely out, it seems, which avoids any weird intermediate steps. I'm not certain whether you folks are already taking this into account, but glancing at the code I suspect we're going to need to stay at least somewhat mime type aware in //net. Specifically, I'm not sure I see a way to pull the logic at the beginning of FixupEncodingTypes that clears the encoding types if the mime type is Application*Gzip and the content-encoding is gzip. If that's true, it is worthwhile trying to pull out a subset of the mime dependencies that have to do with the IsSupported* functions?
On 2015/01/29 01:46:09, rdsmith wrote: > My inclination is to > think that we should check to see whether any servers are actually doing that > with some UMA, and if they're not, remove the code. Ever the conservative. Right, I could read what the code was doing, but it still strikes me as weird. The question is whether we believe the server is in the wrong (and I think we're in agreement it would be) and what the impact would be. The problem with UMA is it means we won't be able to nuke this code for X # of releases, which seems unfortunate for cleanup. I definitely want to nuke, and if it's going to take a while, I think we should look to see if we can't delegate that question up a layer out of //net. How does that smell? > Hmmm. Why do you say it applies only to SDCH? It looks to me like it applies > to gzip as well. Having said that, the histograms have no owners, and I've > never looked at them in the SDCH context, so I'd be happy nuking them from > orbit. Anyone else we should ask? Because they were added for SDCH (source spleunking). Ownerless histograms = die in my book. I'll add bengr@ just to make sure. > I'm not certain whether you folks are already taking this into account, but > glancing at the code I suspect we're going to need to stay at least somewhat > mime type aware in //net. Specifically, I'm not sure I see a way to pull the > logic at the beginning of FixupEncodingTypes that clears the encoding types if > the mime type is Application*Gzip and the content-encoding is gzip. If that's > true, it is worthwhile trying to pull out a subset of the mime dependencies that > have to do with the IsSupported* functions? I'm not sure I understood this question wholly, but the proposal is to just rip out the IsSupported* bits. There are still a number of bits of //net that do need to stay somewhat mime-aware (e.g. sniffing), but don't need to know information about the platform and whether it supports a given mime-type, which seems very much an embedder concern.
rsleevi@chromium.org changed reviewers: + bengr@chromium.org
+bengr for the UMA discussion to see if the Data Reduction Proxy is using these (doubt it)
On 2015/01/29 01:58:19, Ryan Sleevi wrote: > On 2015/01/29 01:46:09, rdsmith wrote: > > My inclination is to > > think that we should check to see whether any servers are actually doing that > > with some UMA, and if they're not, remove the code. > > Ever the conservative. Oh, you know how to hurt a person :-}. But even if I take a step back and think about "move fast and break things", I'm still aware that if servers are being stupid, this could cause a painful UE change, and I don't have any faith that servers aren't stupid. So ... :-J. > Right, I could read what the code was doing, but it still > strikes me as weird. Yeah, sorry, that was mostly me just working it out out loud. > The question is whether we believe the server is in the wrong > (and I think we're in agreement it would be) and what the impact would be. > > The problem with UMA is it means we won't be able to nuke this code for X # of > releases, which seems unfortunate for cleanup. I definitely want to nuke, and > if it's going to take a while, I think we should look to see if we can't > delegate > that question up a layer out of //net. How does that smell? I'd be very solidly on board with that. In an ideal world, we'd centralize the download/no-download decision in one frigging place. I just suspect that's going to require a moderate amount of refactoring. > > Hmmm. Why do you say it applies only to SDCH? It looks to me like it applies > > to gzip as well. Having said that, the histograms have no owners, and I've > > never looked at them in the SDCH context, so I'd be happy nuking them from > > orbit. Anyone else we should ask? > > Because they were added for SDCH (source spleunking). Ownerless histograms = > die in my book. I'll add bengr@ just to make sure. > > > I'm not certain whether you folks are already taking this into account, but > > glancing at the code I suspect we're going to need to stay at least somewhat > > mime type aware in //net. Specifically, I'm not sure I see a way to pull the > > logic at the beginning of FixupEncodingTypes that clears the encoding types if > > the mime type is Application*Gzip and the content-encoding is gzip. If that's > > true, it is worthwhile trying to pull out a subset of the mime dependencies > that > > have to do with the IsSupported* functions? > > I'm not sure I understood this question wholly, but the proposal is to just > rip out the IsSupported* bits. There are still a number of bits of //net that do > need > to stay somewhat mime-aware (e.g. sniffing), but don't need to know information > about the platform and whether it supports a given mime-type, which seems very > much > an embedder concern. SG. My question was mostly "Are we making the distinction at an appropriate conceptual boundary?"--at first I thought the distinction was just "is it in a function or is it examining mime types directly". If the distinction is "What does the underlying platform support?" (or even "What mime-types can the browser natively display?") I think it makes sense to delegate that to the browser.
On 2015/01/29 18:47:54, rdsmith wrote: > On 2015/01/29 01:58:19, Ryan Sleevi wrote: > > On 2015/01/29 01:46:09, rdsmith wrote: > > > My inclination is to > > > think that we should check to see whether any servers are actually doing > that > > > with some UMA, and if they're not, remove the code. > > > > Ever the conservative. > > Oh, you know how to hurt a person :-}. But even if I take a step back and think > about "move fast and break things", I'm still aware that if servers are being > stupid, this could cause a painful UE change, and I don't have any faith that > servers aren't stupid. So ... :-J. > > > Right, I could read what the code was doing, but it still > > strikes me as weird. > > Yeah, sorry, that was mostly me just working it out out loud. > > > The question is whether we believe the server is in the wrong > > (and I think we're in agreement it would be) and what the impact would be. > > > > The problem with UMA is it means we won't be able to nuke this code for X # of > > releases, which seems unfortunate for cleanup. I definitely want to nuke, and > > if it's going to take a while, I think we should look to see if we can't > > delegate > > that question up a layer out of //net. How does that smell? > > I'd be very solidly on board with that. In an ideal world, we'd centralize the > download/no-download decision in one frigging place. I just suspect that's > going to require a moderate amount of refactoring. > > > > Hmmm. Why do you say it applies only to SDCH? It looks to me like it > applies > > > to gzip as well. Having said that, the histograms have no owners, and I've > > > never looked at them in the SDCH context, so I'd be happy nuking them from > > > orbit. Anyone else we should ask? > > > > Because they were added for SDCH (source spleunking). Ownerless histograms = > > die in my book. I'll add bengr@ just to make sure. > > > > > I'm not certain whether you folks are already taking this into account, but > > > glancing at the code I suspect we're going to need to stay at least somewhat > > > mime type aware in //net. Specifically, I'm not sure I see a way to pull > the > > > logic at the beginning of FixupEncodingTypes that clears the encoding types > if > > > the mime type is Application*Gzip and the content-encoding is gzip. If > that's > > > true, it is worthwhile trying to pull out a subset of the mime dependencies > > that > > > have to do with the IsSupported* functions? > > > > I'm not sure I understood this question wholly, but the proposal is to just > > rip out the IsSupported* bits. There are still a number of bits of //net that > do > > need > > to stay somewhat mime-aware (e.g. sniffing), but don't need to know > information > > about the platform and whether it supports a given mime-type, which seems very > > much > > an embedder concern. > > SG. My question was mostly "Are we making the distinction at an appropriate > conceptual boundary?"--at first I thought the distinction was just "is it in a > function or is it examining mime types directly". If the distinction is "What > does the underlying platform support?" (or even "What mime-types can the browser > natively display?") I think it makes sense to delegate that to the browser. Sorry, "... makes sense to delegate that to the *embedder*."
On 2015/01/29 18:49:44, rdsmith wrote: > On 2015/01/29 18:47:54, rdsmith wrote: > > On 2015/01/29 01:58:19, Ryan Sleevi wrote: > > > On 2015/01/29 01:46:09, rdsmith wrote: > > > > My inclination is to > > > > think that we should check to see whether any servers are actually doing > > that > > > > with some UMA, and if they're not, remove the code. > > > > > > Ever the conservative. > > > > Oh, you know how to hurt a person :-}. But even if I take a step back and > think > > about "move fast and break things", I'm still aware that if servers are being > > stupid, this could cause a painful UE change, and I don't have any faith that > > servers aren't stupid. So ... :-J. > > > > > Right, I could read what the code was doing, but it still > > > strikes me as weird. > > > > Yeah, sorry, that was mostly me just working it out out loud. > > > > > The question is whether we believe the server is in the wrong > > > (and I think we're in agreement it would be) and what the impact would be. > > > > > > The problem with UMA is it means we won't be able to nuke this code for X # > of > > > releases, which seems unfortunate for cleanup. I definitely want to nuke, > and > > > if it's going to take a while, I think we should look to see if we can't > > > delegate > > > that question up a layer out of //net. How does that smell? > > > > I'd be very solidly on board with that. In an ideal world, we'd centralize > the > > download/no-download decision in one frigging place. I just suspect that's > > going to require a moderate amount of refactoring. > > > > > > Hmmm. Why do you say it applies only to SDCH? It looks to me like it > > applies > > > > to gzip as well. Having said that, the histograms have no owners, and > I've > > > > never looked at them in the SDCH context, so I'd be happy nuking them from > > > > orbit. Anyone else we should ask? > > > > > > Because they were added for SDCH (source spleunking). Ownerless histograms = > > > die in my book. I'll add bengr@ just to make sure. > > > > > > > I'm not certain whether you folks are already taking this into account, > but > > > > glancing at the code I suspect we're going to need to stay at least > somewhat > > > > mime type aware in //net. Specifically, I'm not sure I see a way to pull > > the > > > > logic at the beginning of FixupEncodingTypes that clears the encoding > types > > if > > > > the mime type is Application*Gzip and the content-encoding is gzip. If > > that's > > > > true, it is worthwhile trying to pull out a subset of the mime > dependencies > > > that > > > > have to do with the IsSupported* functions? > > > > > > I'm not sure I understood this question wholly, but the proposal is to just > > > rip out the IsSupported* bits. There are still a number of bits of //net > that > > do > > > need > > > to stay somewhat mime-aware (e.g. sniffing), but don't need to know > > information > > > about the platform and whether it supports a given mime-type, which seems > very > > > much > > > an embedder concern. > > > > SG. My question was mostly "Are we making the distinction at an appropriate > > conceptual boundary?"--at first I thought the distinction was just "is it in a > > function or is it examining mime types directly". If the distinction is "What > > does the underlying platform support?" (or even "What mime-types can the > browser > > natively display?") I think it makes sense to delegate that to the browser. > > Sorry, "... makes sense to delegate that to the *embedder*." That's a very useful discussion, thanks Ryan and Randy. Btw, Randy, I'm not very familiar with this area, but I assumed the reason for using mime type check in net::Filter was slightly different from what you've described ("so that the user doesn't get a .gz file on their disk that isn't actually gzipped"). I thought it was to handle the case when a user clicks on a link and we get back let's say a file named 'file.txt.gz' with mime type 'text/plain', so that we would automatically uncompress and show the text content of the file directly in the browser, instead of downloading and saving file.txt.gz on disk. But I might be wrong about this, haven't looked deeply. Anyway, given all the gotchas we are discovering here, I'd like to bring back the idea of making a separate CL for moving as much stuff from net/ as possible. Let's keep this CL focused on only moving the media stuff to media/ - it will already make things a bit simpler to untangle and reason about, and would be useful by itself. Ryan, what do you think?
On 2015/01/29 19:02:38, servolk wrote: > On 2015/01/29 18:49:44, rdsmith wrote: > > On 2015/01/29 18:47:54, rdsmith wrote: > > > On 2015/01/29 01:58:19, Ryan Sleevi wrote: > > > > On 2015/01/29 01:46:09, rdsmith wrote: > > > > > My inclination is to > > > > > think that we should check to see whether any servers are actually doing > > > that > > > > > with some UMA, and if they're not, remove the code. > > > > > > > > Ever the conservative. > > > > > > Oh, you know how to hurt a person :-}. But even if I take a step back and > > think > > > about "move fast and break things", I'm still aware that if servers are > being > > > stupid, this could cause a painful UE change, and I don't have any faith > that > > > servers aren't stupid. So ... :-J. > > > > > > > Right, I could read what the code was doing, but it still > > > > strikes me as weird. > > > > > > Yeah, sorry, that was mostly me just working it out out loud. > > > > > > > The question is whether we believe the server is in the wrong > > > > (and I think we're in agreement it would be) and what the impact would be. > > > > > > > > The problem with UMA is it means we won't be able to nuke this code for X > # > > of > > > > releases, which seems unfortunate for cleanup. I definitely want to nuke, > > and > > > > if it's going to take a while, I think we should look to see if we can't > > > > delegate > > > > that question up a layer out of //net. How does that smell? > > > > > > I'd be very solidly on board with that. In an ideal world, we'd centralize > > the > > > download/no-download decision in one frigging place. I just suspect that's > > > going to require a moderate amount of refactoring. > > > > > > > > Hmmm. Why do you say it applies only to SDCH? It looks to me like it > > > applies > > > > > to gzip as well. Having said that, the histograms have no owners, and > > I've > > > > > never looked at them in the SDCH context, so I'd be happy nuking them > from > > > > > orbit. Anyone else we should ask? > > > > > > > > Because they were added for SDCH (source spleunking). Ownerless histograms > = > > > > die in my book. I'll add bengr@ just to make sure. > > > > > > > > > I'm not certain whether you folks are already taking this into account, > > but > > > > > glancing at the code I suspect we're going to need to stay at least > > somewhat > > > > > mime type aware in //net. Specifically, I'm not sure I see a way to > pull > > > the > > > > > logic at the beginning of FixupEncodingTypes that clears the encoding > > types > > > if > > > > > the mime type is Application*Gzip and the content-encoding is gzip. If > > > that's > > > > > true, it is worthwhile trying to pull out a subset of the mime > > dependencies > > > > that > > > > > have to do with the IsSupported* functions? > > > > > > > > I'm not sure I understood this question wholly, but the proposal is to > just > > > > rip out the IsSupported* bits. There are still a number of bits of //net > > that > > > do > > > > need > > > > to stay somewhat mime-aware (e.g. sniffing), but don't need to know > > > information > > > > about the platform and whether it supports a given mime-type, which seems > > very > > > > much > > > > an embedder concern. > > > > > > SG. My question was mostly "Are we making the distinction at an appropriate > > > conceptual boundary?"--at first I thought the distinction was just "is it in > a > > > function or is it examining mime types directly". If the distinction is > "What > > > does the underlying platform support?" (or even "What mime-types can the > > browser > > > natively display?") I think it makes sense to delegate that to the browser. > > > > Sorry, "... makes sense to delegate that to the *embedder*." > > That's a very useful discussion, thanks Ryan and Randy. Btw, Randy, I'm not very > familiar with this area, but I assumed the reason for using mime type check in > net::Filter was slightly different from what you've described ("so that the user > doesn't get a .gz file on their disk that isn't actually gzipped"). I thought it > was to handle the case when a user clicks on a link and we get back let's say a > file named 'file.txt.gz' with mime type 'text/plain', so that we would > automatically uncompress and show the text content of the file directly in the > browser, instead of downloading and saving file.txt.gz on disk. But I might be > wrong about this, haven't looked deeply. You are right, but there are two additional important pieces: 1) This isn't relevant unless the message also has Encoding-Type: gzip (because otherwise encoding_types->clear() won't do anything), and if that's there in the case you're describing, the server is lying to us, because the server hasn't done any encoding at all. If the server wasn't lying, it would always make sense to decode the information, because the actually content the server was attempting to share would have an *extra* encoding added to it that we need to remove. 2) We don't clear the encoding types unless !IsSupportedMimeType(), and the reason we don't clear the encoding types in that case is that we know that if the mime type isn't supported, the apparently-non-download request is going to get turned into a download later in BufferredResourceHandler, and in that case we want to copy the file directly rather than decoding it so the users can see it. > > Anyway, given all the gotchas we are discovering here, I'd like to bring back > the idea of making a separate CL for moving as much stuff from net/ as possible. > Let's keep this CL focused on only moving the media stuff to media/ - it will > already make things a bit simpler to untangle and reason about, and would be > useful by itself. Ryan, what do you think?
We have 4 top-level consumers using this code in different ways; it seems like a strong candidate for a new base/mime directory that everyone could depend on. Has this been considered before? Seems it would obviate any layering concerns.
On 2015/01/29 19:27:04, DaleCurtis wrote: > We have 4 top-level consumers using this code in different ways; it seems like a > strong candidate for a new base/mime directory that everyone could depend on. > Has this been considered before? Seems it would obviate any layering concerns. I think the argument is that, to the extent we can manage it, //net shouldn't be dependent on per-platform or per-browser information about what mime types are supported. That's separate from whether we can make the layering work--we obviously can (since the mime type doesn't have really any dependencies underneath it). It's just the wrong layering. But Ryan's the better layering correctness person, so if he disagrees with me, listen to him.
On 2015/01/29 19:31:26, rdsmith wrote: > I think the argument is that, to the extent we can manage it, //net shouldn't be > dependent on per-platform or per-browser information about what mime types are > supported. That's separate from whether we can make the layering work--we > obviously can (since the mime type doesn't have really any dependencies > underneath it). It's just the wrong layering. But Ryan's the better layering > correctness person, so if he disagrees with me, listen to him. +1. That's exactly correct. //net should not need to know about platform capabilities - just parsing. On 2015/01/29 19:02:38, servolk wrote: > Anyway, given all the gotchas we are discovering here, I'd like to bring back > the idea of making a separate CL for moving as much stuff from net/ as possible. > Let's keep this CL focused on only moving the media stuff to media/ - it will > already make things a bit simpler to untangle and reason about, and would be > useful by itself. Ryan, what do you think? I think this still makes me uncomfortable. I would rather we do the hard work up front - working out the dependencies and layering and solving these - prior to us doing the moving. My very real, concrete fear - one that is supported by ample evidence within Chrome and Google - is that someone will do "just enough" to unblock them, make the pain point disappear - except it will leave the code even more confusing and complicated. We are abundantly guilty of that in //net (the old way is deprecated, the new way is not yet ready, and there's no documentation to help you use either), and I fear that same situation with mime types. I feel like we have a rather defined problem - two callsites - that just require someone thinking carefully about how to extract, delete, or delegate. I don't have a good sense of ownership for this code - I'd like to nominate randy, only because it's !me, but that's not entirely helpful either. I hope this doesn't come off as too depressing or dismissive - I think we absolutely need to do this. We just need to do it right, and that's going to be hard and take time. I'd be thrilled if you want to own this and drive the code, and if not, then I suspect it will "eventually" have to be moved into a //net code health hackathon (Randy, anyone in CAM clamoring for defined-but-tricky problems to work on in //net?) To be explicit: I want to see us break dependencies, then move, rather than move some dependencies but not all.
On 2015/01/29 20:59:01, Ryan Sleevi wrote: > On 2015/01/29 19:31:26, rdsmith wrote: > > I think the argument is that, to the extent we can manage it, //net shouldn't > be > > dependent on per-platform or per-browser information about what mime types are > > supported. That's separate from whether we can make the layering work--we > > obviously can (since the mime type doesn't have really any dependencies > > underneath it). It's just the wrong layering. But Ryan's the better layering > > correctness person, so if he disagrees with me, listen to him. > > +1. That's exactly correct. //net should not need to know about platform > capabilities - just parsing. > > On 2015/01/29 19:02:38, servolk wrote: > > Anyway, given all the gotchas we are discovering here, I'd like to bring back > > the idea of making a separate CL for moving as much stuff from net/ as > possible. > > Let's keep this CL focused on only moving the media stuff to media/ - it will > > already make things a bit simpler to untangle and reason about, and would be > > useful by itself. Ryan, what do you think? > > I think this still makes me uncomfortable. I would rather we do the hard work up > front - working out the dependencies and layering and solving these - prior to > us doing the moving. My very real, concrete fear - one that is supported by > ample evidence within Chrome and Google - is that someone will do "just enough" > to unblock them, make the pain point disappear - except it will leave the code > even more confusing and complicated. We are abundantly guilty of that in //net > (the old way is deprecated, the new way is not yet ready, and there's no > documentation to help you use either), and I fear that same situation with mime > types. > > I feel like we have a rather defined problem - two callsites - that just require > someone thinking carefully about how to extract, delete, or delegate. I don't > have a good sense of ownership for this code - I'd like to nominate randy, only > because it's !me, but that's not entirely helpful either. > > I hope this doesn't come off as too depressing or dismissive - I think we > absolutely need to do this. We just need to do it right, and that's going to be > hard and take time. I'd be thrilled if you want to own this and drive the code, > and if not, then I suspect it will "eventually" have to be moved into a //net > code health hackathon (Randy, anyone in CAM clamoring for defined-but-tricky > problems to work on in //net?) > > To be explicit: I want to see us break dependencies, then move, rather than move > some dependencies but not all. I'm a good person to nominate in terms of knowledge--I know the download code (for my sins) and am supposed to be the filter code expert. I'm not sure I'm a good person to nominate in terms of free cycles. But anyone who attacks this will almost certainly need to coordinate carefully with asanka@ or I (for the download code interaction), and arguably it should be one of us.
On 2015/01/29 21:18:10, rdsmith wrote: > On 2015/01/29 20:59:01, Ryan Sleevi wrote: > > On 2015/01/29 19:31:26, rdsmith wrote: > > > I think the argument is that, to the extent we can manage it, //net > shouldn't > > be > > > dependent on per-platform or per-browser information about what mime types > are > > > supported. That's separate from whether we can make the layering work--we > > > obviously can (since the mime type doesn't have really any dependencies > > > underneath it). It's just the wrong layering. But Ryan's the better > layering > > > correctness person, so if he disagrees with me, listen to him. > > > > +1. That's exactly correct. //net should not need to know about platform > > capabilities - just parsing. > > > > On 2015/01/29 19:02:38, servolk wrote: > > > Anyway, given all the gotchas we are discovering here, I'd like to bring > back > > > the idea of making a separate CL for moving as much stuff from net/ as > > possible. > > > Let's keep this CL focused on only moving the media stuff to media/ - it > will > > > already make things a bit simpler to untangle and reason about, and would be > > > useful by itself. Ryan, what do you think? > > > > I think this still makes me uncomfortable. I would rather we do the hard work > up > > front - working out the dependencies and layering and solving these - prior to > > us doing the moving. My very real, concrete fear - one that is supported by > > ample evidence within Chrome and Google - is that someone will do "just > enough" > > to unblock them, make the pain point disappear - except it will leave the code > > even more confusing and complicated. We are abundantly guilty of that in //net > > (the old way is deprecated, the new way is not yet ready, and there's no > > documentation to help you use either), and I fear that same situation with > mime > > types. > > > > I feel like we have a rather defined problem - two callsites - that just > require > > someone thinking carefully about how to extract, delete, or delegate. I don't > > have a good sense of ownership for this code - I'd like to nominate randy, > only > > because it's !me, but that's not entirely helpful either. > > > > I hope this doesn't come off as too depressing or dismissive - I think we > > absolutely need to do this. We just need to do it right, and that's going to > be > > hard and take time. I'd be thrilled if you want to own this and drive the > code, > > and if not, then I suspect it will "eventually" have to be moved into a //net > > code health hackathon (Randy, anyone in CAM clamoring for defined-but-tricky > > problems to work on in //net?) Whoops, sorry, missed this question. Not that I know of. > > To be explicit: I want to see us break dependencies, then move, rather than > move > > some dependencies but not all. > > I'm a good person to nominate in terms of knowledge--I know the download code > (for my sins) and am supposed to be the filter code expert. I'm not sure I'm a > good person to nominate in terms of free cycles. But anyone who attacks this > will almost certainly need to coordinate carefully with asanka@ or I (for the > download code interaction), and arguably it should be one of us.
On 2015/01/29 21:18:53, rdsmith wrote: > On 2015/01/29 21:18:10, rdsmith wrote: > > On 2015/01/29 20:59:01, Ryan Sleevi wrote: > > > On 2015/01/29 19:31:26, rdsmith wrote: > > > > I think the argument is that, to the extent we can manage it, //net > > shouldn't > > > be > > > > dependent on per-platform or per-browser information about what mime types > > are > > > > supported. That's separate from whether we can make the layering work--we > > > > obviously can (since the mime type doesn't have really any dependencies > > > > underneath it). It's just the wrong layering. But Ryan's the better > > layering > > > > correctness person, so if he disagrees with me, listen to him. > > > > > > +1. That's exactly correct. //net should not need to know about platform > > > capabilities - just parsing. > > > > > > On 2015/01/29 19:02:38, servolk wrote: > > > > Anyway, given all the gotchas we are discovering here, I'd like to bring > > back > > > > the idea of making a separate CL for moving as much stuff from net/ as > > > possible. > > > > Let's keep this CL focused on only moving the media stuff to media/ - it > > will > > > > already make things a bit simpler to untangle and reason about, and would > be > > > > useful by itself. Ryan, what do you think? > > > > > > I think this still makes me uncomfortable. I would rather we do the hard > work > > up > > > front - working out the dependencies and layering and solving these - prior > to > > > us doing the moving. My very real, concrete fear - one that is supported by > > > ample evidence within Chrome and Google - is that someone will do "just > > enough" > > > to unblock them, make the pain point disappear - except it will leave the > code > > > even more confusing and complicated. We are abundantly guilty of that in > //net > > > (the old way is deprecated, the new way is not yet ready, and there's no > > > documentation to help you use either), and I fear that same situation with > > mime > > > types. > > > > > > I feel like we have a rather defined problem - two callsites - that just > > require > > > someone thinking carefully about how to extract, delete, or delegate. I > don't > > > have a good sense of ownership for this code - I'd like to nominate randy, > > only > > > because it's !me, but that's not entirely helpful either. > > > > > > I hope this doesn't come off as too depressing or dismissive - I think we > > > absolutely need to do this. We just need to do it right, and that's going to > > be > > > hard and take time. I'd be thrilled if you want to own this and drive the > > code, > > > and if not, then I suspect it will "eventually" have to be moved into a > //net > > > code health hackathon (Randy, anyone in CAM clamoring for defined-but-tricky > > > problems to work on in //net?) > > Whoops, sorry, missed this question. Not that I know of. > > > > To be explicit: I want to see us break dependencies, then move, rather than > > move > > > some dependencies but not all. > > > > I'm a good person to nominate in terms of knowledge--I know the download code > > (for my sins) and am supposed to be the filter code expert. I'm not sure I'm > a > > good person to nominate in terms of free cycles. But anyone who attacks this > > will almost certainly need to coordinate carefully with asanka@ or I (for the > > download code interaction), and arguably it should be one of us. I'm late to the party. Are you asking if the data reduction proxy says "Content-Encoding: gzip" for a resource that's gzipped? What exactly is the concern?
On 2015/01/29 22:52:26, bengr wrote: > I'm late to the party. Are you asking if the data reduction proxy says > "Content-Encoding: gzip" for a resource that's gzipped? What exactly is the > concern? Ben: Does the DRP care about either of the two behaviors identified - the histograms for compression rates (added for SDCH, but possibly repurposed by DRP) or the weird case of tar.gz with a gzip encoding.
On 2015/01/30 00:48:56, Ryan Sleevi wrote: > On 2015/01/29 22:52:26, bengr wrote: > > I'm late to the party. Are you asking if the data reduction proxy says > > "Content-Encoding: gzip" for a resource that's gzipped? What exactly is the > > concern? > > Ben: Does the DRP care about either of the two behaviors identified - the > histograms for compression rates (added for SDCH, but possibly repurposed by > DRP) or the weird case of tar.gz with a gzip encoding. No.
servolk@chromium.org changed reviewers: + ddorwin@chromium.org
Ok, so after our recent discussions, I've rebased this CL onto ToT. Please take another look. This CL extracts media mime type checks and moves them to media/base/mime_util* Previous warning about net::IsSupportedNonImageMimeType no longer including media mime types still applies.
https://codereview.chromium.org/877323009/diff/160001/content/browser/frame_h... File content/browser/frame_host/navigation_controller_impl.cc (left): https://codereview.chromium.org/877323009/diff/160001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl.cc:492: !net::IsSupportedMediaMimeType(mime_type); This change is clearly a change in behaviour and logic. Why is this correct?
https://codereview.chromium.org/877323009/diff/160001/content/browser/frame_h... File content/browser/frame_host/navigation_controller_impl.cc (left): https://codereview.chromium.org/877323009/diff/160001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl.cc:492: !net::IsSupportedMediaMimeType(mime_type); On 2015/02/25 03:04:24, Ryan Sleevi wrote: > This change is clearly a change in behaviour and logic. > > Why is this correct? You are talking about IsSupportedNonImageMimeType behavior and logic, right? Aaron's original CL (https://codereview.chromium.org/401523002/) did that too and so I assumed that's the right thing to do. And after looking at its usage some more, I'm inclined to agree, and in fact I'd prefer to remove the IsSupportedNonImageMimeType completely (although that would probably be done in a separate CL). As far as I can tell this function is not formally specified anywhere, it's a legacy that we got from WebKit. Any code that really needs to get a subset of supported mime media types that are not images should be able to do that trivially (IsSupportedMimeType && !IsSupportedImageMimeType). A few places that are/were using IsSupportedNonImageMimeType either excluded media types explicitly (like here) or incorrectly assumed that media mime types are excluded from the set of non-image types. e.g. the URLRequestHttpJob::IsCompressibleContent used to assume that, in the UMA code that I recently removed, see crbug.com/456916. And another instance (still active) where code doesn't expect media types to be included into non-image types can be found at https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... Do we have any strong reasons to keep that function and/or preserve it's behavior exactly as it was in the past?
https://codereview.chromium.org/877323009/diff/160001/content/browser/frame_h... File content/browser/frame_host/navigation_controller_impl.cc (left): https://codereview.chromium.org/877323009/diff/160001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl.cc:492: !net::IsSupportedMediaMimeType(mime_type); On 2015/02/25 23:39:31, servolk wrote: > On 2015/02/25 03:04:24, Ryan Sleevi wrote: > > This change is clearly a change in behaviour and logic. > > > > Why is this correct? > > You are talking about IsSupportedNonImageMimeType behavior and logic, right?' No. The very clear deletion of "IsSupportedMediaMimeType" Why is it appropriate to no longer check that? I realize I may be coming across as short, but we seem to be having frequent communication failures, so I'm being forced to pay extra attention to these CLs. It would help if you can explain or justify logic whenever you're changing the behaviour of code as to why it's OK to change.
On 2015/02/25 23:43:13, Ryan Sleevi wrote: > https://codereview.chromium.org/877323009/diff/160001/content/browser/frame_h... > File content/browser/frame_host/navigation_controller_impl.cc (left): > > https://codereview.chromium.org/877323009/diff/160001/content/browser/frame_h... > content/browser/frame_host/navigation_controller_impl.cc:492: > !net::IsSupportedMediaMimeType(mime_type); > On 2015/02/25 23:39:31, servolk wrote: > > On 2015/02/25 03:04:24, Ryan Sleevi wrote: > > > This change is clearly a change in behaviour and logic. > > > > > > Why is this correct? > > > > You are talking about IsSupportedNonImageMimeType behavior and logic, right?' > > No. The very clear deletion of "IsSupportedMediaMimeType" > > Why is it appropriate to no longer check that? > > I realize I may be coming across as short, but we seem to be having frequent > communication failures, so I'm being forced to pay extra attention to these CLs. > It would help if you can explain or justify logic whenever you're changing the > behaviour of code as to why it's OK to change. Because this change modifies the behavior of IsSupportedNonImageMimeType. Specifically, before this change net::IsSupportedNonImageMimeType used to return true for supported media mime types (since media mime types were added into net::MimeUtil::non_image_map_ by MimeUtil::InitializeMimeTypeMaps). But now that we are moving mime type checks to another file we can no longer do that (take a look at the changes done in net/base/mime_util.cc and the new media/base/mime_util.cc), we'll have a separate media::MimeUtil in the media/base/mime_util.cc that will have complete knowledge of media types. So net::IsSupportedNonImageMimeType will now return false for media types and so we don't need to exclude them explicitly here anymore. As to why that's actually a good thing - I've tried to explain in my previous comment. Hope that answers your question, I actually appreciate someone more familiar with net/ paying attention to this CL. If you have more questions/concerns - feel free to let me know and let's discuss.
Thanks for the explanation, totally makes sense now. I'd ask that you rework the commit comment - I think a lot of the explanation bits are relevant to the CL as a reviewable piece, but not to the commit as a part of source history. That is, I think you could probably delete everything but the summary and still be good, but it might help to just document the change in behaviour e.g. Extract media mime type checks from net/base into media/base This moves the processing of media mime types into //media, as //net does not need to know about them. Note: This changes the behaviour of net::IsSupportedNonImageMimeType, which would previously return true for media types. All of the callers expecting that behaviour are updated in this CL. Or something like that. The rest is context/discussion meant for the reviewers. https://codereview.chromium.org/877323009/diff/160001/media/base/mime_util.cc File media/base/mime_util.cc (right): https://codereview.chromium.org/877323009/diff/160001/media/base/mime_util.cc... media/base/mime_util.cc:654: Everything above here should be in an unnamed namespace to avoid any potential ODR violations.
Once those two concerns (ODR and CL description) are addressed, LGTM
On 2015/02/26 01:00:45, Ryan Sleevi wrote: > Once those two concerns (ODR and CL description) are addressed, LGTM Thanks for looking Ryan! Unfortunately it looks like I'll have some more changes to this CL though (bringing it even closer to the Aaron's original CL). After looking at trybot results, I've noticed that this change would also remove media mime types from the set of mime types reported as supported by net::IsSupportedMimeType (the generic one). This caused some unit test breaks under chrome/browser/. Here is what I'm planning to do in order to deal with that: 1. I'm going to introduce content::IsSupportedMimeType already in this CL (by adding content/public/common/mime_util*) which will correspond to the old/current net::IsSupportedMimeType. 2. I'll update all the places that currently use net::IsSupportedMimeType to use content::IsSupportedMimeType to make sure we fully preserve the past behavior. 3. The only place which can't be updated is net::Filter, but that code doesn't care about media and is going to go away eventually. 4. I think I'll also rename net::IsSupportedMimeType into net::IsSupportedMimeType_old or net::IsSupportedMimeType_deprecated. We'll remove it eventually, but in a follow-up CL, since this CL is again growing pretty large. Sorry for the last minute changes, but I feel like that's the only sane way to move forward. Any comments/objections - please let me know.
On 2015/02/26 02:38:16, servolk wrote: > On 2015/02/26 01:00:45, Ryan Sleevi wrote: > > Once those two concerns (ODR and CL description) are addressed, LGTM > > Thanks for looking Ryan! Unfortunately it looks like I'll have some more changes > to this CL though (bringing it even closer to the Aaron's original CL). After > looking at trybot results, I've noticed that this change would also remove media > mime types from the set of mime types reported as supported by > net::IsSupportedMimeType (the generic one). This caused some unit test breaks > under chrome/browser/. Here is what I'm planning to do in order to deal with > that: > 1. I'm going to introduce content::IsSupportedMimeType already in this CL (by > adding content/public/common/mime_util*) which will correspond to the > old/current net::IsSupportedMimeType. > 2. I'll update all the places that currently use net::IsSupportedMimeType to use > content::IsSupportedMimeType to make sure we fully preserve the past behavior. > 3. The only place which can't be updated is net::Filter, but that code doesn't > care about media and is going to go away eventually. > 4. I think I'll also rename net::IsSupportedMimeType into > net::IsSupportedMimeType_old or net::IsSupportedMimeType_deprecated. We'll > remove it eventually, but in a follow-up CL, since this CL is again growing > pretty large. > Sorry for the last minute changes, but I feel like that's the only sane way to > move forward. > Any comments/objections - please let me know. Oh bother. I should have paid closer attention. You changing IsSupportedMediaMimeType breaks the //net Filter behaviour, which we've explicitly agreed and stated multiple times is not acceptable. I'm going to have to ask you to not land this (so not LGTM) until we get things sorted. To put differently: Please cease all refactoring work until we gather the data sufficient to remove this behaviour. This is why we had a meeting. This is why we have a plan.
On 2015/02/26 02:38:16, servolk wrote: > On 2015/02/26 01:00:45, Ryan Sleevi wrote: > > Once those two concerns (ODR and CL description) are addressed, LGTM > > Thanks for looking Ryan! Unfortunately it looks like I'll have some more changes > to this CL though (bringing it even closer to the Aaron's original CL). After > looking at trybot results, I've noticed that this change would also remove media > mime types from the set of mime types reported as supported by > net::IsSupportedMimeType (the generic one). This caused some unit test breaks > under chrome/browser/. Here is what I'm planning to do in order to deal with > that: > 1. I'm going to introduce content::IsSupportedMimeType already in this CL (by > adding content/public/common/mime_util*) which will correspond to the > old/current net::IsSupportedMimeType. > 2. I'll update all the places that currently use net::IsSupportedMimeType to use > content::IsSupportedMimeType to make sure we fully preserve the past behavior. > 3. The only place which can't be updated is net::Filter, but that code doesn't > care about media and is going to go away eventually. > 4. I think I'll also rename net::IsSupportedMimeType into > net::IsSupportedMimeType_old or net::IsSupportedMimeType_deprecated. We'll > remove it eventually, but in a follow-up CL, since this CL is again growing > pretty large. > Sorry for the last minute changes, but I feel like that's the only sane way to > move forward. > Any comments/objections - please let me know. Oh bother. I should have paid closer attention. You changing IsSupportedMediaMimeType breaks the //net Filter behaviour, which we've explicitly agreed and stated multiple times is not acceptable. I'm going to have to ask you to not land this (so not LGTM) until we get things sorted. To put differently: Please cease all refactoring work until we gather the data sufficient to remove this behaviour. This is why we had a meeting. This is why we have a plan.
On 2015/02/26 05:03:37, Ryan Sleevi wrote: > On 2015/02/26 02:38:16, servolk wrote: > > On 2015/02/26 01:00:45, Ryan Sleevi wrote: > > > Once those two concerns (ODR and CL description) are addressed, LGTM > > > > Thanks for looking Ryan! Unfortunately it looks like I'll have some more > changes > > to this CL though (bringing it even closer to the Aaron's original CL). After > > looking at trybot results, I've noticed that this change would also remove > media > > mime types from the set of mime types reported as supported by > > net::IsSupportedMimeType (the generic one). This caused some unit test breaks > > under chrome/browser/. Here is what I'm planning to do in order to deal with > > that: > > 1. I'm going to introduce content::IsSupportedMimeType already in this CL (by > > adding content/public/common/mime_util*) which will correspond to the > > old/current net::IsSupportedMimeType. > > 2. I'll update all the places that currently use net::IsSupportedMimeType to > use > > content::IsSupportedMimeType to make sure we fully preserve the past behavior. > > 3. The only place which can't be updated is net::Filter, but that code doesn't > > care about media and is going to go away eventually. > > 4. I think I'll also rename net::IsSupportedMimeType into > > net::IsSupportedMimeType_old or net::IsSupportedMimeType_deprecated. We'll > > remove it eventually, but in a follow-up CL, since this CL is again growing > > pretty large. > > Sorry for the last minute changes, but I feel like that's the only sane way to > > move forward. > > Any comments/objections - please let me know. > > Oh bother. I should have paid closer attention. You changing > IsSupportedMediaMimeType breaks the //net Filter behaviour, which we've > explicitly agreed and stated multiple times is not acceptable. > > I'm going to have to ask you to not land this (so not LGTM) until we get things > sorted. > > To put differently: Please cease all refactoring work until we gather the data > sufficient to remove this behaviour. This is why we had a meeting. This is why > we have a plan. Ok, Ryan, I understand your concerns. But I would like to point out that I think the major problem with the current "plan" is that there's absolutely no ETA for when this is going to be done. It's been more than 6 months since the last attempt by Aaron to solve this, and yet we are still where we began. I'm not the only person who is being held back by the fact that media mime type checks are under net/, other people on media/ team are also affected (see crbug.com/318217). And in fact there has been all sorts of discussions about doing this for 3 years (!) see crbug.com/163800, but still no progress. So I will postpone this CL for now, but can you at least help us get some traction on this long-standing painful issue from net/ side? Could you help us find somebody on the net/ team who can prioritize removing the old dependency from net::Filter and unblock the path for moving the mime type checks to content/ where they belong?
> Could you help us find somebody > on the net/ team who can prioritize removing the old dependency from net::Filter > and unblock the path for moving the mime type checks to content/ where they > belong? As we said during the meeting, Asanka is looking at this for M43. Neither of the bugs you linked are relevant to demonstrating the issues you have, and as we also said _during the meeting_, if there is anything blocking you (besides, and I wish it didn't bear repeating, *this* bug, which explicitly said during the meeting //net would own), that I'm happy to review and get through.
On 2015/02/26 20:26:03, Ryan Sleevi wrote: > > Could you help us find somebody > > on the net/ team who can prioritize removing the old dependency from > net::Filter > > and unblock the path for moving the mime type checks to content/ where they > > belong? > > As we said during the meeting, Asanka is looking at this for M43. > > Neither of the bugs you linked are relevant to demonstrating the issues you > have, and as we also said _during the meeting_, if there is anything blocking > you (besides, and I wish it didn't bear repeating, *this* bug, which explicitly > said during the meeting //net would own), that I'm happy to review and get > through. To give you a better example of the issues that this is causing, take a look at dalecurtis@ comments on this CL: https://codereview.chromium.org/816353010/ (and the same kind of issue is also blocking CL https://codereview.chromium.org/812643005/) In short: when adding new media codecs we sometimes need to have them gated by a gyp variable (to not get Chrome into trouble for containing code that deals with proprietary codecs, that might require licensing fees and other legal ramifications). That would be easy to do by just adding a new gyp flag to media/media.gyp. But since at the moment media mime type checks live under net/, if we wanted to have a single option/flag controlling that, we would be forced to add such a flag to some place that would be visible from both net/ and media/. Currently the only scope I'm aware of is build/common.gypi (see for example enable_mpeg2ts_stream_parser flag in build/common.gypi), but adding new flags to build/common.gypi has very high costs, since it affects a lot of code and various configurations. Any ideas how to solve that short of moving mime type checks into media/ ?
On 2015/02/26 20:37:32, servolk wrote: > On 2015/02/26 20:26:03, Ryan Sleevi wrote: > > > Could you help us find somebody > > > on the net/ team who can prioritize removing the old dependency from > > net::Filter > > > and unblock the path for moving the mime type checks to content/ where they > > > belong? > > > > As we said during the meeting, Asanka is looking at this for M43. > > > > Neither of the bugs you linked are relevant to demonstrating the issues you > > have, and as we also said _during the meeting_, if there is anything blocking > > you (besides, and I wish it didn't bear repeating, *this* bug, which > explicitly > > said during the meeting //net would own), that I'm happy to review and get > > through. > > To give you a better example of the issues that this is causing, take a look at > dalecurtis@ comments on this CL: > https://codereview.chromium.org/816353010/ > (and the same kind of issue is also blocking CL > https://codereview.chromium.org/812643005/) > In short: when adding new media codecs we sometimes need to have them gated by a > gyp variable (to not get Chrome into trouble for containing code that deals with > proprietary codecs, that might require licensing fees and other legal > ramifications). That would be easy to do by just adding a new gyp flag to > media/media.gyp. But since at the moment media mime type checks live under net/, > if we wanted to have a single option/flag controlling that, we would be forced > to add such a flag to some place that would be visible from both net/ and > media/. Currently the only scope I'm aware of is build/common.gypi (see for > example enable_mpeg2ts_stream_parser flag in build/common.gypi), but adding new > flags to build/common.gypi has very high costs, since it affects a lot of code > and various configurations. Any ideas how to solve that short of moving mime > type checks into media/ ? Thank you both for working to clarify the results of our meeting. In https://codereview.chromium.org/816353010/#msg7, Dale pointed out one way to unblock that CL (without blocking on lifting media stuff out of //net): duplicate that CL's new GYP variable in both media.gyp and net.gyp (and not have it in common.gypi). Though unpalatable, especially for maintenance, it might be viable for short-term.
On 2015/02/26 21:33:42, wolenetz wrote: > On 2015/02/26 20:37:32, servolk wrote: > > On 2015/02/26 20:26:03, Ryan Sleevi wrote: > > > > Could you help us find somebody > > > > on the net/ team who can prioritize removing the old dependency from > > > net::Filter > > > > and unblock the path for moving the mime type checks to content/ where > they > > > > belong? > > > > > > As we said during the meeting, Asanka is looking at this for M43. > > > > > > Neither of the bugs you linked are relevant to demonstrating the issues you > > > have, and as we also said _during the meeting_, if there is anything > blocking > > > you (besides, and I wish it didn't bear repeating, *this* bug, which > > explicitly > > > said during the meeting //net would own), that I'm happy to review and get > > > through. > > > > To give you a better example of the issues that this is causing, take a look > at > > dalecurtis@ comments on this CL: > > https://codereview.chromium.org/816353010/ > > (and the same kind of issue is also blocking CL > > https://codereview.chromium.org/812643005/) > > In short: when adding new media codecs we sometimes need to have them gated by > a > > gyp variable (to not get Chrome into trouble for containing code that deals > with > > proprietary codecs, that might require licensing fees and other legal > > ramifications). That would be easy to do by just adding a new gyp flag to > > media/media.gyp. But since at the moment media mime type checks live under > net/, > > if we wanted to have a single option/flag controlling that, we would be forced > > to add such a flag to some place that would be visible from both net/ and > > media/. Currently the only scope I'm aware of is build/common.gypi (see for > > example enable_mpeg2ts_stream_parser flag in build/common.gypi), but adding > new > > flags to build/common.gypi has very high costs, since it affects a lot of code > > and various configurations. Any ideas how to solve that short of moving mime > > type checks into media/ ? > > Thank you both for working to clarify the results of our meeting. > In https://codereview.chromium.org/816353010/#msg7, Dale pointed out one way to > unblock that CL (without blocking on lifting media stuff out of //net): > duplicate that CL's new GYP variable in both media.gyp and net.gyp (and not have > it in common.gypi). Though unpalatable, especially for maintenance, it might be > viable for short-term. Thanks Matthew. Yes, as a temporary work around we could have two separate flags in net/ and media/, and while certainly ugly from maintenance point of view this could allow us to work around the issue to some extent. But I'd also like to get explicit approval from Ryan, as one of net/OWNERS to go ahead with this approach (since we'd have to add this flag for net/ .gyp files), and I'd also like to give net/ folks better idea of the issues we have to motivate them to finally solve this. Also I'd like to point out that since we want to support various codecs besides HEVC, this would translate into flags for each codec to be duplicated in net/ and media/, which, again, is doable, but makes it even less appealing from maintenance point of view. But if this is preferred over extracting the mime type checks to media/, sure we can go down this path.
That was the idea I suggested during the meeting and Ryan seemed okay with; given the net/ team's commitments I think it's the best approach for now.
On 2015/02/26 21:54:01, DaleCurtis wrote: > That was the idea I suggested during the meeting and Ryan seemed okay with; > given the net/ team's commitments I think it's the best approach for now. Yes, we're totally committed to help you find short-term solutions and long-term solutions for the problems. On the issue with .GYP files, I can totally understand Brett's reluctance, but I do not take it to be a blanket ban. I take his frustration as being similar to mine - we should work harder to find general solutions, while also being careful introducing many permutations of build configurations (since we can only "support" so many on the waterfall) On a matter of GYP/GN health, having a project have variables local to their GYP - even just in //media - is a *recipe* for disaster. So at the risk of placing you between a rock and a hard place, where you want to *go* for //media is (in my opinion) worse than what was tried (for build/common.gypi) That is, you're treating the symptom, rather than treating the root cause (exploding permutations of build configurations) To the specific question you asked, putting a GYP flag just in //net and //media targets is a recipe for bad build health, so I will push back on that. However, the alternative, putting it in build/common.gypi, is also generally bad for build health. That's why you're getting negative signals on both ends. Perhaps it's better to take up this broader issue of proprietary codecs in a bug, but we already have a flag for dealing with these codecs - the "proprietary_codecs" flag. If you need more nuance than this, understanding what the nuance is (e.g. *this* codec is only enabled on Chromecast builds, for example) may help provide suggestions on how you can accomplish that _without_ introducing new build flags. I'm happy to consult on GYP/GN build health, since I totally want you to be able to land your code.
On 2015/02/26 23:32:34, Ryan Sleevi wrote: > On 2015/02/26 21:54:01, DaleCurtis wrote: > > That was the idea I suggested during the meeting and Ryan seemed okay with; > > given the net/ team's commitments I think it's the best approach for now. > > Yes, we're totally committed to help you find short-term solutions and long-term > solutions for the problems. > > On the issue with .GYP files, I can totally understand Brett's reluctance, but I > do not take it to be a blanket ban. I take his frustration as being similar to > mine - we should work harder to find general solutions, while also being careful > introducing many permutations of build configurations (since we can only > "support" so many on the waterfall) > > On a matter of GYP/GN health, having a project have variables local to their GYP > - even just in //media - is a *recipe* for disaster. So at the risk of placing > you between a rock and a hard place, where you want to *go* for //media is (in > my opinion) worse than what was tried (for build/common.gypi) > > That is, you're treating the symptom, rather than treating the root cause > (exploding permutations of build configurations) > > To the specific question you asked, putting a GYP flag just in //net and //media > targets is a recipe for bad build health, so I will push back on that. However, > the alternative, putting it in build/common.gypi, is also generally bad for > build health. That's why you're getting negative signals on both ends. > > Perhaps it's better to take up this broader issue of proprietary codecs in a > bug, but we already have a flag for dealing with these codecs - the > "proprietary_codecs" flag. If you need more nuance than this, understanding what > the nuance is (e.g. *this* codec is only enabled on Chromecast builds, for > example) may help provide suggestions on how you can accomplish that _without_ > introducing new build flags. > > I'm happy to consult on GYP/GN build health, since I totally want you to be able > to land your code. Thanks, that a lot of useful feedback! Of course I don't want to cause maintenance issues for anybody, but I don't know how to satisfy all the requirements from various folks simultaneously. I'll try to explain the issue a bit more, hopefully this will get us closer to solving this. Let's take the specific example of adding HEVC support. Chromecast and Android need to be able to support HEVC, so we do need to have this code added to media/ and net/. But on the other hand there are concerns about HEVC being proprietary and potentially requiring a separate license, so we would like to have the parts of code that handle HEVC to be gated by a separate gyp flag. Using just proprietary_codecs flag is not enough, because public releases of the Chrome browser are built and shipped with proprietary_codecs=1 and we still don't want Chrome to contain any HEVC-related code yet to avoid any potential licensing requirements (and situation with AC3/EAC3/DTS is very-very similar). That's why we had to introduce a separate enable_hevc_demuxing flag. I don't see any other way around it, do you? And now that we have this flag, we have to put it somewhere. build/common.gypi is one possible place, but according to build/ maintainers that leads to configuration explosion - although perhaps there's a way to limit that to some extent? I can see that there are already separate "chromecast", "android", "chromeos" variables. Is there a way to add global configuration flags limited to just those platforms? I was adding the new HEVC flag in a manner similar to the already existing enable_mpeg2ts_stream_parser flag - but that's defined at the top level outside of all specific platforms, and as far as I understand that's exactly what we want to avoid. Does that explain all the necessary details? Any ideas how to achieve what we want without introducing new build global flags?
On 2015/02/27 00:17:19, servolk wrote: > Let's take the specific example of adding HEVC support. Chromecast and Android > need to be able to support HEVC, so we do need to have this code added to media/ > and net/. But on the other hand there are concerns about HEVC being proprietary > and potentially requiring a separate license, so we would like to have the parts > of code that handle HEVC to be gated by a separate gyp flag. Using just > proprietary_codecs flag is not enough, because public releases of the Chrome > browser are built and shipped with proprietary_codecs=1 and we still don't want > Chrome to contain any HEVC-related code yet to avoid any potential licensing > requirements (and situation with AC3/EAC3/DTS is very-very similar). That's why > we had to introduce a separate enable_hevc_demuxing flag. I don't see any other > way around it, do you? in //net and //media 'conditions': [ ['proprietary_codecs == 1 and (chromecast == 1 || OS == "android")', { 'defines': 'USE_SOMETHING_HERE', }], ], That #define may ONLY be used in .cc files. It MUST NOT be used in any .h files, period. > And now that we have this flag, we have to put it somewhere. build/common.gypi > is one possible place, but according to build/ maintainers that leads to > configuration explosion - although perhaps there's a way to limit that to some > extent? I think you've failed to catch the salient point of my reply - which is you're fundamentally doing this. That's why Brett's concerned. Introducing a build flag _at all_ contributes to that explosion, whether it's in //net or in //media or in //build/common.gypi. I absolutely agree that a flag is literally the place of last resort.
On 2015/02/27 00:32:25, Ryan Sleevi wrote: > On 2015/02/27 00:17:19, servolk wrote: > > Let's take the specific example of adding HEVC support. Chromecast and Android > > need to be able to support HEVC, so we do need to have this code added to > media/ > > and net/. But on the other hand there are concerns about HEVC being > proprietary > > and potentially requiring a separate license, so we would like to have the > parts > > of code that handle HEVC to be gated by a separate gyp flag. Using just > > proprietary_codecs flag is not enough, because public releases of the Chrome > > browser are built and shipped with proprietary_codecs=1 and we still don't > want > > Chrome to contain any HEVC-related code yet to avoid any potential licensing > > requirements (and situation with AC3/EAC3/DTS is very-very similar). That's > why > > we had to introduce a separate enable_hevc_demuxing flag. I don't see any > other > > way around it, do you? > > in //net and //media > 'conditions': [ > ['proprietary_codecs == 1 and (chromecast == 1 || OS == "android")', { > 'defines': 'USE_SOMETHING_HERE', > }], > ], > > That #define may ONLY be used in .cc files. It MUST NOT be used in any .h files, > period. Could you elaborate on why we can only use it in .cc? If you look at HEVC CL (https://codereview.chromium.org/816353010/), we need to add a new supported mp4 box type (HEVCDecoderConfigurationRecord in media/formats/mp4/box_definitions.h), which can only be present in HEVC streams. It needs to be in the header file, because it's being used to convey decoder parameters into MP4StreamParser::PrepareHEVCBuffer where we use it to convert input data buffer into AnnexB format, which is what most decoders expect. I think I partly understand where the restriction to use the define only in .cc stems from (since .h potentially can be included outside of net/ or media/ and it wouldn't have this definition there), but at least for the internal headers can we lift that restriction? > > And now that we have this flag, we have to put it somewhere. build/common.gypi > > is one possible place, but according to build/ maintainers that leads to > > configuration explosion - although perhaps there's a way to limit that to some > > extent? > > I think you've failed to catch the salient point of my reply - which is you're > fundamentally doing this. That's why Brett's concerned. Introducing a build flag > _at all_ contributes to that explosion, whether it's in //net or in //media or > in //build/common.gypi. > > I absolutely agree that a flag is literally the place of last resort. Well, as I've explained I was doing my changes following what's already been done before me with enable_mpeg2ts_stream_parser flag, I assumed doing something analogous to the existing flag should be fine. But it turns out I was wrong and we need to find a better way going forward (and I assume we'll need to fix the old enable_mpeg2ts_stream_parser flag as well, once we find the right way). That's why this discussion is very valuable to me - I hope with your deeper understanding of those gyp flags and configuration issues we'll be able to find the right way to do this sooner that I could do on my own. I'm relatively new to Chrome/Chromium, so your suggestions on how to do this right are very appreciated.
On 2015/02/27 01:00:53, servolk wrote: > Could you elaborate on why we can only use it in .cc? > If you look at HEVC CL (https://codereview.chromium.org/816353010/), we need to > add a new supported mp4 box type (HEVCDecoderConfigurationRecord in > media/formats/mp4/box_definitions.h), which can only be present in HEVC streams. > It needs to be in the header file, because it's being used to convey decoder > parameters into MP4StreamParser::PrepareHEVCBuffer where we use it to convert > input data buffer into AnnexB format, which is what most decoders expect. I > think I partly understand where the restriction to use the define only in .cc > stems from (since .h potentially can be included outside of net/ or media/ and > it wouldn't have this definition there), but at least for the internal headers > can we lift that restriction? GYP doesn't make a distinction between public/private headers, and GN hasn't fleshed it out. The only place in Chrome we really have public/private headers is in content. Almost every single month since I've started at Google, there's been someone who has botched a dependency in a build file and introduced build flakiness. If you're using a #define in a header, and someone #include's it in a target that isn't properly expressed via the GYP dependencies (and please note, NONE of our GYP files are fully expressive of their dependencies, and Brett's been having a fair bit of trouble getting GN to do that, although he IS working on it for GN), it then potentially causes a security issue (e.g. if the object size changes), and not just a compile error (or worse, compile flake) So for any sort of define or feature condition that needs to be used across multiple targets, it needs to be in build/common.gypi However, that said, you *still have* the information you need, and can get away WITHOUT introducing the convenience #define I was suggesting, if you absolutely have to. In that case, in your .h #if defined(USE_PROPRIETARY_CODECS) .. enable the demuxing #endif I am still at a loss, even with your explanations, about why proprietary_codecs isn't enough to express, well, this proprietary codec. That's why I'm trying to gather the requirements to see what can be suggested, and to understand why some of the existing methods were discounted - was it intentional to overlook these (and I"m just not understanding why), or was it just that it wasn't realized the cleaner way of doing things. > Well, as I've explained I was doing my changes following what's already been > done before me with enable_mpeg2ts_stream_parser flag, I assumed doing something > analogous to the existing flag should be fine. But it turns out I was wrong and > we need to find a better way going forward (and I assume we'll need to fix the > old enable_mpeg2ts_stream_parser flag as well, once we find the right way). Yes. Apologies that no one yelled sooner :)
On 2015/02/27 02:17:02, Ryan Sleevi wrote: > On 2015/02/27 01:00:53, servolk wrote: > > Could you elaborate on why we can only use it in .cc? > > If you look at HEVC CL (https://codereview.chromium.org/816353010/), we need > to > > add a new supported mp4 box type (HEVCDecoderConfigurationRecord in > > media/formats/mp4/box_definitions.h), which can only be present in HEVC > streams. > > It needs to be in the header file, because it's being used to convey decoder > > parameters into MP4StreamParser::PrepareHEVCBuffer where we use it to convert > > input data buffer into AnnexB format, which is what most decoders expect. I > > think I partly understand where the restriction to use the define only in .cc > > stems from (since .h potentially can be included outside of net/ or media/ and > > it wouldn't have this definition there), but at least for the internal headers > > can we lift that restriction? > > GYP doesn't make a distinction between public/private headers, and GN hasn't > fleshed it out. The only place in Chrome we really have public/private headers > is in content. > > Almost every single month since I've started at Google, there's been someone who > has botched a dependency in a build file and introduced build flakiness. If > you're using a #define in a header, and someone #include's it in a target that > isn't properly expressed via the GYP dependencies (and please note, NONE of our > GYP files are fully expressive of their dependencies, and Brett's been having a > fair bit of trouble getting GN to do that, although he IS working on it for GN), > it then potentially causes a security issue (e.g. if the object size changes), > and not just a compile error (or worse, compile flake) > > So for any sort of define or feature condition that needs to be used across > multiple targets, it needs to be in build/common.gypi > > However, that said, you *still have* the information you need, and can get away > WITHOUT introducing the convenience #define I was suggesting, if you absolutely > have to. > > In that case, in your .h > > #if defined(USE_PROPRIETARY_CODECS) > .. enable the demuxing > #endif > > I am still at a loss, even with your explanations, about why proprietary_codecs > isn't enough to express, well, this proprietary codec. That's why I'm trying to > gather the requirements to see what can be suggested, and to understand why some > of the existing methods were discounted - was it intentional to overlook these > (and I"m just not understanding why), or was it just that it wasn't realized the > cleaner way of doing things. > > > Well, as I've explained I was doing my changes following what's already been > > done before me with enable_mpeg2ts_stream_parser flag, I assumed doing > something > > analogous to the existing flag should be fine. But it turns out I was wrong > and > > we need to find a better way going forward (and I assume we'll need to fix the > > old enable_mpeg2ts_stream_parser flag as well, once we find the right way). > > Yes. Apologies that no one yelled sooner :) Wait, sorry, let me explain a bit more. Yes, HEVC is proprietary codec. But you have to keep in mind, that Chrome browser is built and shipped to the end-users with proprietary_codecs=1 (to support other more common proprietary codecs - H264, AAC, etc). So we can't just use proprietary_codecs=1 to toggle HEVC support, we need a separate flag. Otherwise Chrome would start claiming it supports HEVC (via HTMLMediaElement::canPlayType), which is not true, Chrome doesn't currently include software decoders for HEVC. So the actual condition for enabling HEVC should be proprietary_codecs=1 && enable_hevc_demuxing=1. Then regular Chrome builds will have proprietary_codecs=1,enable_hevc_demuxing=0 and will not include any HEVC-related code and will (correctly) report that HEVC is unsupported via canPlayType. While platforms that have hardware HEVC decoders (Chromecast, Android) will use have proprietary_codecs=1,enable_hevc_demuxing=1 and will support HEVC. Oh, yeah, and another reason why it's hard to avoid having #if defined(ENABLE_HEVC_SUPPORT) in .h header files - HEVC, also known as H265, is essentially the next version of H264 codec, and is based heavily on it, and shares like 90% of code with H264/MP4 code path. I've tried to avoid unnecessary code duplication as much as possible, which is why, for example, I added those little bits of new code to the existing mp4 parser, instead of creating a new mp4 parser for HEVC support, it would be almost completely the same code except for those small differences currently wrapped into #if defined(ENABLE_HEVC_DEMUXING). So, to be honest, I still don't fully understand how your suggestion would solve this - how can we avoid using the new define in .h files?
On 2015/02/27 21:35:01, servolk wrote: > Wait, sorry, let me explain a bit more. Yes, HEVC is proprietary codec. But you > have to keep in mind, that Chrome browser is built and shipped to the end-users > with proprietary_codecs=1 (to support other more common proprietary codecs - > H264, AAC, etc). So we can't just use proprietary_codecs=1 to toggle HEVC > support, we need a separate flag. Otherwise Chrome would start claiming it > supports HEVC (via HTMLMediaElement::canPlayType), which is not true, Chrome > doesn't currently include software decoders for HEVC. So the actual condition > for enabling HEVC should be proprietary_codecs=1 && enable_hevc_demuxing=1. Then > regular Chrome builds will have proprietary_codecs=1,enable_hevc_demuxing=0 and > will not include any HEVC-related code and will (correctly) report that HEVC is > unsupported via canPlayType. While platforms that have hardware HEVC decoders > (Chromecast, Android) will use have proprietary_codecs=1,enable_hevc_demuxing=1 > and will support HEVC. > Oh, yeah, and another reason why it's hard to avoid having #if > defined(ENABLE_HEVC_SUPPORT) in .h header files - HEVC, also known as H265, is > essentially the next version of H264 codec, and is based heavily on it, and > shares like 90% of code with H264/MP4 code path. I've tried to avoid unnecessary > code duplication as much as possible, which is why, for example, I added those > little bits of new code to the existing mp4 parser, instead of creating a new > mp4 parser for HEVC support, it would be almost completely the same code except > for those small differences currently wrapped into #if > defined(ENABLE_HEVC_DEMUXING). > So, to be honest, I still don't fully understand how your suggestion would solve > this - how can we avoid using the new define in .h files? Alas, I feel we're talking past eachother again, and I doubt a meeting is going to solve this. Indirectly, you answered my question, which is enable_hevc_demuxing == (proprietary_codecs=1 && (chromecast == 1 || OS == "Android")) *that* is what you should be using in your GYP files. You should NOT be introducing an additional flag. For your headers, rather than #if defined(ENABLE_HEVC_DEMUXING) You should be using #if defined(USE_PROPRIETARY_CODECS) && (defined(OS_ANDROID) || defined(whatever_chromecast_uses)) Why does this all matter? Because the whole point is *not* to introduce another permutation of a build config. What does it mean to enable_hevc_demuxing is proprietary_codecs==0, for example? Or if OS=="Windows". They're *not* supported configs, so it's an invalid build config.
On 2015/02/27 21:45:27, Ryan Sleevi wrote: > On 2015/02/27 21:35:01, servolk wrote: > > Wait, sorry, let me explain a bit more. Yes, HEVC is proprietary codec. But > you > > have to keep in mind, that Chrome browser is built and shipped to the > end-users > > with proprietary_codecs=1 (to support other more common proprietary codecs - > > H264, AAC, etc). So we can't just use proprietary_codecs=1 to toggle HEVC > > support, we need a separate flag. Otherwise Chrome would start claiming it > > supports HEVC (via HTMLMediaElement::canPlayType), which is not true, Chrome > > doesn't currently include software decoders for HEVC. So the actual condition > > for enabling HEVC should be proprietary_codecs=1 && enable_hevc_demuxing=1. > Then > > regular Chrome builds will have proprietary_codecs=1,enable_hevc_demuxing=0 > and > > will not include any HEVC-related code and will (correctly) report that HEVC > is > > unsupported via canPlayType. While platforms that have hardware HEVC decoders > > (Chromecast, Android) will use have > proprietary_codecs=1,enable_hevc_demuxing=1 > > and will support HEVC. > > Oh, yeah, and another reason why it's hard to avoid having #if > > defined(ENABLE_HEVC_SUPPORT) in .h header files - HEVC, also known as H265, is > > essentially the next version of H264 codec, and is based heavily on it, and > > shares like 90% of code with H264/MP4 code path. I've tried to avoid > unnecessary > > code duplication as much as possible, which is why, for example, I added those > > little bits of new code to the existing mp4 parser, instead of creating a new > > mp4 parser for HEVC support, it would be almost completely the same code > except > > for those small differences currently wrapped into #if > > defined(ENABLE_HEVC_DEMUXING). > > So, to be honest, I still don't fully understand how your suggestion would > solve > > this - how can we avoid using the new define in .h files? > > Alas, I feel we're talking past eachother again, and I doubt a meeting is going > to solve this. Indirectly, you answered my question, which is > > enable_hevc_demuxing == (proprietary_codecs=1 && (chromecast == 1 || OS == > "Android")) > > *that* is what you should be using in your GYP files. You should NOT be > introducing an additional flag. > > For your headers, rather than > #if defined(ENABLE_HEVC_DEMUXING) > > You should be using > #if defined(USE_PROPRIETARY_CODECS) && (defined(OS_ANDROID) || > defined(whatever_chromecast_uses)) > > Why does this all matter? > Because the whole point is *not* to introduce another permutation of a build > config. What does it mean to enable_hevc_demuxing is proprietary_codecs==0, for > example? Or if OS=="Windows". They're *not* supported configs, so it's an > invalid build config. This approach does not seem scalable to me from the maintenance point of view. For example today we are ready to support HEVC on Chromecast. We go ahead and surround pieces of HEVC-related code with #if defined(USE_PROPRIETARY_CODECS) && defined(CHROMECAST_BUILD). Tomorrow Android wants to add HEVC support - what are they supposed to do, go and update each #if condition scattered all over the place? Then, sometime in the future Chrome decides to support HEVC via, let's say external DXVA API on Windows or VDAPI on Mac OS X - they's also have to go and update all the #if conditions everywhere. The whole point of having a gyp configuration flag is that gives you this flexibility to turn features on and off in one place in the gyp file. Whereas the opposite problem that you presented seems relatively small to me. The configuration space is well defined and won't lead to any breakage. You have proprietary_codecs=0? you don't get any proprietary codecs (including HEVC) on any platforms, but that shouldn't break the build (I'll admit that it might with the current version of HEVC CL, but at least I can see how to easily make it work that way, and I'm planning to do that soon). Same for OS==Windows. You build with OS==Windows && proprietary_codecs=1 && enable_hevc_demuxing=1? You get HEVC demuxing support. But you have to keep in mind, that enable_hevc_demuxing is 0 by default. If you explicitly set it to 1 you presumably know what you are doing (and it's your responsibility then to make sure you have all the other necessary prerequisites for this to work, e.g. you've added a DXVA-based HEVC decoder for Windows). Just adding a new configuration flag shouldn't break anything, as long as it's set to 0 by default.
On Fri, Feb 27, 2015 at 1:45 PM, <rsleevi@chromium.org> wrote: > On 2015/02/27 21:35:01, servolk wrote: > >> Wait, sorry, let me explain a bit more. Yes, HEVC is proprietary codec. >> But >> > you > >> have to keep in mind, that Chrome browser is built and shipped to the >> > end-users > >> with proprietary_codecs=1 (to support other more common proprietary >> codecs - >> H264, AAC, etc). So we can't just use proprietary_codecs=1 to toggle HEVC >> support, we need a separate flag. Otherwise Chrome would start claiming it >> supports HEVC (via HTMLMediaElement::canPlayType), which is not true, >> Chrome >> doesn't currently include software decoders for HEVC. So the actual >> condition >> for enabling HEVC should be proprietary_codecs=1 && >> enable_hevc_demuxing=1. >> > Then > >> regular Chrome builds will have proprietary_codecs=1,enable_ >> hevc_demuxing=0 >> > and > >> will not include any HEVC-related code and will (correctly) report that >> HEVC >> > is > >> unsupported via canPlayType. While platforms that have hardware HEVC >> decoders >> (Chromecast, Android) will use have >> > proprietary_codecs=1,enable_hevc_demuxing=1 > >> and will support HEVC. >> Oh, yeah, and another reason why it's hard to avoid having #if >> defined(ENABLE_HEVC_SUPPORT) in .h header files - HEVC, also known as >> H265, is >> essentially the next version of H264 codec, and is based heavily on it, >> and >> shares like 90% of code with H264/MP4 code path. I've tried to avoid >> > unnecessary > >> code duplication as much as possible, which is why, for example, I added >> those >> little bits of new code to the existing mp4 parser, instead of creating a >> new >> mp4 parser for HEVC support, it would be almost completely the same code >> > except > >> for those small differences currently wrapped into #if >> defined(ENABLE_HEVC_DEMUXING). >> So, to be honest, I still don't fully understand how your suggestion would >> > solve > >> this - how can we avoid using the new define in .h files? >> > > Alas, I feel we're talking past eachother again, and I doubt a meeting is > going > to solve this. Indirectly, you answered my question, which is > > enable_hevc_demuxing == (proprietary_codecs=1 && (chromecast == 1 || OS == > "Android")) > > *that* is what you should be using in your GYP files. You should NOT be > introducing an additional flag. > > For your headers, rather than > #if defined(ENABLE_HEVC_DEMUXING) > > You should be using > #if defined(USE_PROPRIETARY_CODECS) && (defined(OS_ANDROID) || > defined(whatever_chromecast_uses)) > Hi Ryan, Unfortunately I don't think we were able to use a Chromecast specific C macro in the code as the use of this "whatever_chromecast_uses" macro was simply shot down when we first started to upstream our code. We used to have this "CHROMECAST_BUILD" macro all over the place in chromium/src in our downstream fork. When we started our upstream effort in late 2013 and early last year, both Darin (he is already on this email thread) and John (jam@) didn't want us to introduce yet another new platform macro. What they suggested back then is that if there is anything that specific to Chromecast but not applicable to Linux desktop, we should try to create a feature-based gyp variable and/or C macros. This way, the code for that feature is not strictly tied to a particular platform but something that each platform can decide to turn on or off. We were so hoping that we could introduce this "CHROMECAST_BUILD" macro in the code as that would have made our life much easier back then. :-) But after working on and sticking to this feature-based approach, we found it's actually not quite bad for our upstream effort, and in fact, helps the overall chromium source cleanness as we often could reduce the code like this #if defined(OS_ANDROID) || defined (OS_CHROMEOS) || defined(OS_IOS) ||defined(CHROMECAST_BUILD) into something like #if defined(FEATURE_BASED_MACRO) > > Why does this all matter? > Because the whole point is *not* to introduce another permutation of a > build > config. What does it mean to enable_hevc_demuxing is > proprietary_codecs==0, for > example? Or if OS=="Windows". They're *not* supported configs, so it's an > invalid build config. > > https://codereview.chromium.org/877323009/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
I still feel like we're not making progress on here, but I'm trying to find where and how best to communicate the issues. Since it appears to have been misread several times, considering that servolk@ is going back to the same argument: - The goal is to have as few build variables and permutations as possible. This is true whether we're talking build variables or defines. We want as few of those as possible. Sometimes, it's unavoidable - use_proprietary_codecs is a great example of this - but on the whole, we want Chromium to build as Chromium, not a Chinese menu of options that you pick and choose. That's not to say we don't offer mix-and-match options at all - //components is a great example of this - but we restrict it at compile time and not generally through build flags. To answer servolk@'s explicit question - are they supposed to update it in all those places when adding a new platform - yes, the answer is yes. I understand why you're arguing for a central flag (and I address it below), but I want to be explicit and unambiguous that yes, it can be more desirable to require _that_ than allow a single GYP variable to be flipped. I'm quite familiar with the issues lcwu@ mentions about Chromecast upstreaming. I was and am 100% onboard with the advice Darin communicated to that team, and worked to help that be done for //net. I'm certainly happy to continue helping here. Now, servolk@, it's clear from talking with you and some of your fellow //media stack owners that the "HEVC on Mac/Windows/etc" is not yet decided. So it feels very much that you're presenting a strawman, and I think it's unfair and unreasonable to expect a response to that. It's also something that, if you do decide to go down that path, we can certainly change code and structure to match whatever long-term goal there is. But we shouldn't prematurely complexify things for the "what if" case. We should focus on what we have. What we have is the HEVC bits only used for - Proprietary codecs - For Android and ChromeCast We're clear it doesn't make sense without also setting proprietary codecs, and it also doesn't make sense setting for one of the platforms that isn't supported. Those two things (depends on another flag, doesn't work on some platforms) is _generally_ a sign when and where a GYP variable is undesirable. The _ideal_ solution is you would inject those bits, at compile time, with no variables needed. You'd make your target depend on those hevc bits. This is what was advised for Chromecast for //net code. It required restructuring, and rethinking how the code was laid out, but in the end, it did well. The next best solution is that you keep this logic out of _any_ header files, and solely localized to .cc files. Then you can make target-specific decisions in a single .gyp that does the thing you need to do for the platforms you need. In that .gyp file, you only use existing build flags - don't allow mix-and-match permutations. Now, if you need to use a symbol in a header, or you need it to span multiple targets, then it belongs in a global place. - If you need to use the symbol in a header, it belongs in a global define, at least in the GYP world. - This is getting fixed in GN, but until the strict deps are enforced, we can't assume it's fixed yet - For GYP, 'export_dependent_settings' is not sufficient practice, even though it's totally the right mechanism - One way to get a global define is centralizing your logic in //build/build_config.h - Another way is to explicitly set it via //build/common.gypi - If you need to use a variable across multiple targets, then it belongs in //build/common.gypi - You can propagate that variable to your *C++ files ONLY* by having target-specific conditional defines - You SHOULD NOT use any such define in any header. It's too fragile and difficult for people to maintain. Since you need this visible across //net and //media *AND* because you need it visible in //media headers, //build/common.gypi + a define seems like the only option, and you can and should find a //build owner to discuss this. If you refactored //media to get rid of the .h dependency, //build/common.gypi + a variable seems like a viable option. So if Brett blocks the define (and whether or not he would is TBD), then //media would need to explore how it can make the matter of HEVC support an _internal_ API decision, rather than an _external_ visible decision.
and note, all of this is better discussed on a bug. We're far from the CL in question at this point, so please, file a bug or let's continue on it there.
On 2015/02/27 23:18:45, Ryan Sleevi wrote: > I still feel like we're not making progress on here, but I'm trying to find > where and how best to communicate the issues. > > Since it appears to have been misread several times, considering that servolk@ > is going back to the same argument: > - The goal is to have as few build variables and permutations as possible. > > This is true whether we're talking build variables or defines. We want as few of > those as possible. Sometimes, it's unavoidable - use_proprietary_codecs is a > great example of this - but on the whole, we want Chromium to build as Chromium, > not a Chinese menu of options that you pick and choose. That's not to say we > don't offer mix-and-match options at all - //components is a great example of > this - but we restrict it at compile time and not generally through build flags. > > To answer servolk@'s explicit question - are they supposed to update it in all > those places when adding a new platform - yes, the answer is yes. I understand > why you're arguing for a central flag (and I address it below), but I want to be > explicit and unambiguous that yes, it can be more desirable to require _that_ > than allow a single GYP variable to be flipped. > > I'm quite familiar with the issues lcwu@ mentions about Chromecast upstreaming. > I was and am 100% onboard with the advice Darin communicated to that team, and > worked to help that be done for //net. I'm certainly happy to continue helping > here. > > Now, servolk@, it's clear from talking with you and some of your fellow //media > stack owners that the "HEVC on Mac/Windows/etc" is not yet decided. So it feels > very much that you're presenting a strawman, and I think it's unfair and > unreasonable to expect a response to that. It's also something that, if you do > decide to go down that path, we can certainly change code and structure to match > whatever long-term goal there is. But we shouldn't prematurely complexify things > for the "what if" case. We should focus on what we have. > > What we have is the HEVC bits only used for > - Proprietary codecs > - For Android and ChromeCast > > We're clear it doesn't make sense without also setting proprietary codecs, and > it also doesn't make sense setting for one of the platforms that isn't > supported. Those two things (depends on another flag, doesn't work on some > platforms) is _generally_ a sign when and where a GYP variable is undesirable. > > The _ideal_ solution is you would inject those bits, at compile time, with no > variables needed. You'd make your target depend on those hevc bits. This is what > was advised for Chromecast for //net code. It required restructuring, and > rethinking how the code was laid out, but in the end, it did well. > > The next best solution is that you keep this logic out of _any_ header files, > and solely localized to .cc files. Then you can make target-specific decisions > in a single .gyp that does the thing you need to do for the platforms you need. > In that .gyp file, you only use existing build flags - don't allow mix-and-match > permutations. > > Now, if you need to use a symbol in a header, or you need it to span multiple > targets, then it belongs in a global place. > - If you need to use the symbol in a header, it belongs in a global define, at > least in the GYP world. > - This is getting fixed in GN, but until the strict deps are enforced, we > can't assume it's fixed yet > - For GYP, 'export_dependent_settings' is not sufficient practice, even though > it's totally the right mechanism > - One way to get a global define is centralizing your logic in > //build/build_config.h > - Another way is to explicitly set it via //build/common.gypi > - If you need to use a variable across multiple targets, then it belongs in > //build/common.gypi > - You can propagate that variable to your *C++ files ONLY* by having > target-specific conditional defines > - You SHOULD NOT use any such define in any header. It's too fragile and > difficult for people to maintain. > > Since you need this visible across //net and //media *AND* because you need it > visible in //media headers, //build/common.gypi + a define seems like the only > option, and you can and should find a //build owner to discuss this. If you > refactored //media to get rid of the .h dependency, //build/common.gypi + a > variable seems like a viable option. So if Brett blocks the define (and whether > or not he would is TBD), then //media would need to explore how it can make the > matter of HEVC support an _internal_ API decision, rather than an _external_ > visible decision. We may not be making much progress in terms of code changes, but I feel this is a very useful discussion for gaining better understanding of the issues with gyp configuration options. I understand that you have other things to do, but I think we need to find a solution for these issues sooner or later. So feel free to suggest better people to discuss this with (brettw@ ?) and perhaps a better format (move the discussion to Chromium discussion Google Groups?). I'll open a bug per your suggestion, but what should that bug title be? Something like "Need to find a better way to deal with various configuration options on different platforms/devices"? To answer your questions: 1. HEVC support on other platforms is not a strawman. I'm personally 99% sure Chrome will need to support HEVC on Windows/Mac and other platforms, it's only a matter of time. HEVC is _the_ international standard for video, major content providers (Netflix,Vudu,etc) are starting to use it. Some people in Google/Chrome believe vp9/vp10 is a viable alternative. I believe it's a viable alternative only if you care exclusively about YouTube. Other major content provider don't seem as eager to jump on vp9/vp10 train, and it's very hard to fight against the whole world. But that's a topic for a completely separate lengthy discussion, let's not get distracted with that for now. I agree that right now we'll only need HEVC support on Chromecast and Android, but I just wanted to make it clear why not having global config flags scales poorly. 2. There are hundreds of platform-specific gyp flags in build/common.gypi. I gave you one example, I can give you many more (use_clipboard_aurax11, mac_views_browser, win_use_allocator_shim, again, there are dozens if not hundreds of them). If we generally oppose platform-specific options, how come we have so many of them? Is that a recent change in direction? Is there some kind of document providing guidance on this (or at least some kind of summary of previous discussions?). I've explained some of the problems we are trying to solve. Again, being somewhat unfamiliar with gyp until fairly recently I'm just trying to look at what other people did in the past and solve my problems in the same way (which,btw, I think is important for consistency and improving maintainability). Is the move to BUILD.gn system intended to solve this? And btw, I think having a flexible configuration system with the ability to pick and choose options is advantageous. The reason we got hundreds of all those other options is because world is diverse and full of quirky devices and platforms, and we need to be able to deal with that. I think there's nothing wrong with having many config options, as long as they all have reasonable, non-contradicting defaults. It gives you flexibility. Linux kernel does that, major projects I've worked on in the past did that too. And as Chrome keeps growing in scope it'll probably have to do the same. I think the goal should be having a manageable/maintainable solution which supports all the necessary configurations, which is not the same as minimizing the number of build variables. Now again, don't get me wrong, I understand that we can't solve all of those problems at once. But I'd still like to understand what is the reasonable solution to move forward in this case. I'll try discussing adding a new common.gypi flag with brettw@, if he disagrees we'll have to try and find some solution with global headers or something.
On 2015/02/28 00:39:12, servolk wrote: > We may not be making much progress in terms of code changes, but I feel this is > a very useful discussion for gaining better understanding of the issues with gyp > configuration options. I understand that you have other things to do, but I > think we need to find a solution for these issues sooner or later. So feel free > to suggest better people to discuss this with (brettw@ ?) and perhaps a better > format (move the discussion to Chromium discussion Google Groups?). I'll open a > bug per your suggestion, but what should that bug title be? Something like "Need > to find a better way to deal with various configuration options on different > platforms/devices"? Sure > 2. There are hundreds of platform-specific gyp flags in build/common.gypi. I > gave you one example, I can give you many more (use_clipboard_aurax11, > mac_views_browser, win_use_allocator_shim, again, there are dozens if not > hundreds of them). If we generally oppose platform-specific options, how come we > have so many of them? Is that a recent change in direction? Is there some kind > of document providing guidance on this (or at least some kind of summary of > previous discussions?). When? Hardly recent. Why? Tragedy of the commons. Where? It's come up in a variety of threads. Probably the most vocal on this matter has been John Abd-El-Malek ( jam@chromium.org ), on the need of removing configurations that we aren't testing from the waterfall / infrastructure. A build flag that isn't part of the Chromium waterfall is indistinguishable from dead code, and a build config that is on the waterfall is more headache for developers to learn. > Is the move to BUILD.gn system intended to solve this? Somewhat, in a few tangible ways. 1) As I mentioned earlier, Brett is working to make BUILD.gn require fully declarative dependencies, something that GYP _could_ do but never enforced. As a result of it not enforcing this, our GYP files are hyper-minimalist. For example, you can see many projects that lack a dependency on //base, even though they do depend on it. 2) Brett (and the GN folk in general) are also working to ensure that you can set project-local configurations that only affect the direct dependencies, precisely to better support local configurations such at this. > It gives you flexibility. Linux kernel does that, > major projects I've worked on in the past did that too. And as Chrome keeps > growing in scope it'll probably have to do the same. I think the goal should be > having a manageable/maintainable solution which supports all the necessary > configurations, which is not the same as minimizing the number of build > variables. This is, historically, a matter of design that many influential early members of the Chromium project disagree with (and, for the record, I also find myself in that camp). We are generally very aggressive about removing dead code and unsupported configurations, despite there being interest by "some" to support them. Every #ifdef is added code-maintenance burden. Similar arguments are made for user options, which are briefly discussed in http://www.chromium.org/user-experience/options . That is, we try to follow opinionated defaults. We don't support FreeBSD, for example. Similarly, part of the motivation for forking OpenSSL into BoringSSL was to remove the vast accumulation of compile-time options and esoteric platform/configuration settings, and instead support sane defaults universally. I realize these are different in practice than your suggestion, but I highlight these to try to give a sense of the culture behind much of Chromium, which has been "sane by default" and "minimize options" - which goes beyond just our UI and extends well into our code and build systems. > Now again, don't get me wrong, I understand that we can't solve all of those > problems at once. But I'd still like to understand what is the reasonable > solution to move forward in this case. I'll try discussing adding a new > common.gypi flag with brettw@, if he disagrees we'll have to try and find some > solution with global headers or something. The four people who can give you the best feedback (and I would suggest having this discussion with them if you're unsatisfied with the results or wish to know more) thakis jam brettw dpranke Brett and Dirk can help explain GN and GYP best practice (respectively), and Nico and John can help best inculturate where I may have failed.
Is the GYP/GN discussion blocking this specific CL? If so, what specific change is the problem? It looks like the CL was approved until tests caught a regression. Would this CL be acceptable if the regression could be fixed? The regression appears to only be related to container types (and not codecs). The container-related code is much less complex than the codecs code and changes much less frequently. Therefore, duplicating it until a more permanent solution can be developed would not be much of a burden. I think the advantages of getting the codecs code out of net/ and enabling some other work we want to do in media/ would far outweigh this. From a quick review of the code, it appears we would just need to duplicate common_media_types, proprietary_media_types, and IsMimeTypeSupportedOnAndroid() to restore the previous behavior. Thoughts?
On 2015/03/13 18:14:19, ddorwin wrote: > Is the GYP/GN discussion blocking this specific CL? If so, what specific change > is the problem? > > It looks like the CL was approved until tests caught a regression. Would this CL > be acceptable if the regression could be fixed? > > The regression appears to only be related to container types (and not codecs). > The container-related code is much less complex than the codecs code and changes > much less frequently. Therefore, duplicating it until a more permanent solution > can be developed would not be much of a burden. I think the advantages of > getting the codecs code out of net/ and enabling some other work we want to do > in media/ would far outweigh this. > > From a quick review of the code, it appears we would just need to duplicate > common_media_types, proprietary_media_types, and IsMimeTypeSupportedOnAndroid() > to restore the previous behavior. > > Thoughts? Short version: This will have to be postponed for now. After a separate discussion with rsleevi@ and brettw@ about GYP/GN issues, we have worked out a short term solution that will unblock us (Chromecast team). Longer version: As you can see in the comments above, we had a long discussion about this. Everybody seems to agree that mime type checks need to be moved out of net/ eventually. But the thing that prevents this right now is the fact that net::Filter::FixupEncodingTypes calls net::IsSupportedMimeType directly. This dependency has to be broken first, before we can move the mime type checking code somewhere else (since net/ is a low-level library, we don't want net/ to depend either on media/ or content/). rsleevi@ and rdsmith@ in the comments above explained why it might be possible to remove this piece of code completely, but in order to be safe and make sure we don't regress anything (e.g. crbug.com/16430 which made necessary adding this code in the first place) we need to gather some UMA statistics. The problem with this particular CL, that attempted to extract just media mime type checks into media/, is that it breaks net::IsSupportedNonImageType and, more importantly, the generic net::IsSupportedMimeType behavior (these two would no longer report media types as supported and one would have to be aware that it's necessary to check media::IsSupportedMediaMimeType explicitly, in addition to net::IsSupportedMimeType, in places where that makes a difference). I'm not sure fully understand your suggestion on how to fix this regression. Partially duplicating mime type checking code sounds like a bad idea to me, I think the code in net/ and media/ would quickly diverge. But perhaps I'm misunderstanding, feel free to send a patch.
I added the specific proposals inline. See if reverting those 2 or 3 pieces of code fixes the test failures. Extracting this code from net/ is important for the media team, not just for unblocking Chromecast. With so much progress (and need from media/), I'd like to complete it if we can. https://codereview.chromium.org/877323009/diff/200001/net/base/mime_util.cc File net/base/mime_util.cc (right): https://codereview.chromium.org/877323009/diff/200001/net/base/mime_util.cc#n... net/base/mime_util.cc:225: Proposal: 1) Revert this removal of common_media_type and proprietary_media_types. 2) Add a comment here and in the media/ copy to update the other whenever making changes. These lists *rarely* change, so I wouldn't expect there to be any changes before we have a permanent solution or are able to remove the code that depends on this. https://codereview.chromium.org/877323009/diff/200001/net/base/mime_util.cc#n... net/base/mime_util.cc:306: MimeUtil::MimeUtil() { Proposal: Revert the removal of IsMimeTypeSupportedOnAndroid(). However, I'm not sure we need this in either place since ICS is the minimum supported version. In fact, even that is no longer supported: http://blog.chromium.org/2015/03/freezing-chrome-for-ice-cream-sandwich_3.html https://codereview.chromium.org/877323009/diff/200001/net/base/mime_util.cc#n... net/base/mime_util.cc:322: non_image_map_.insert(supported_javascript_types[i]); Proposal: Revert the code that inserts *_media_types into non_image_map_.
On 2015/03/13 20:44:59, ddorwin wrote: > I added the specific proposals inline. See if reverting those 2 or 3 pieces of > code fixes the test failures. > > Extracting this code from net/ is important for the media team, not just for > unblocking Chromecast. With so much progress (and need from media/), I'd like to > complete it if we can. > > https://codereview.chromium.org/877323009/diff/200001/net/base/mime_util.cc > File net/base/mime_util.cc (right): > > https://codereview.chromium.org/877323009/diff/200001/net/base/mime_util.cc#n... > net/base/mime_util.cc:225: > Proposal: > 1) Revert this removal of common_media_type and proprietary_media_types. > 2) Add a comment here and in the media/ copy to update the other whenever making > changes. > > These lists *rarely* change, so I wouldn't expect there to be any changes before > we have a permanent solution or are able to remove the code that depends on > this. > > https://codereview.chromium.org/877323009/diff/200001/net/base/mime_util.cc#n... > net/base/mime_util.cc:306: MimeUtil::MimeUtil() { > Proposal: Revert the removal of IsMimeTypeSupportedOnAndroid(). > > However, I'm not sure we need this in either place since ICS is the minimum > supported version. In fact, even that is no longer supported: > http://blog.chromium.org/2015/03/freezing-chrome-for-ice-cream-sandwich_3.html > > https://codereview.chromium.org/877323009/diff/200001/net/base/mime_util.cc#n... > net/base/mime_util.cc:322: non_image_map_.insert(supported_javascript_types[i]); > Proposal: Revert the code that inserts *_media_types into non_image_map_. It might be trickier than it seems at first. If we just reverted those few changes, but left the rest of the code intact, still how would we deal with having to invoke the actual codec checks from net/, given it can't have dependency on media/ ? That's why I suggested you try creating a patch and testing it yourself :) Besides, as I've already mentioned I think duplicating code is really bad idea, even if we added comments telling people to keep things in sync. In my experience it's too easy to overlook comments, you want something that breaks at compile time if things are out of sync (a bunch of static_asserts perhaps? but I don't have a good idea right now how it would look, somebody would need to experiment with it). If we really want to do this, I think a saner approach might be do to this first: https://codereview.chromium.org/953633002/ This would break the dependency of net/ on mime type checking code. That would allow us to proceed with moving mime type checking code into content/ and media/, without removing any code from net::Filter for now. But Ryan was against that CL. Perhaps you can help me convince him that it's a better plan than waiting for gathering UMA statistics (as far as I know there's no ETA for that from net/ team).
On 2015/03/13 21:07:58, servolk wrote: > > This would break the dependency of net/ on mime type checking code. That would > allow us to proceed with moving mime type checking code into content/ and > media/, without removing any code from net::Filter for now. But Ryan was against > that CL. Perhaps you can help me convince him that it's a better plan than > waiting for gathering UMA statistics (as far as I know there's no ETA for that > from net/ team). We have stated repeatedly - in hangout, in email, and in IM - that this will be a few weeks. That was an explicit ETA we provided and agreed to. Just please go star/read https://code.google.com/p/chromium/issues/detail?id=318217#c9 I will continue to block any CLs that attempt to refactor this code to accomplish some net/ / media/ split until the plan we all agreed to is complete. Please don't take that as an ultimatum or an excuse to find other reviewers - this is something the //net team *is* owning and *is* working on, and I'm getting frustrated having to repeat it :( Cheers
On 2015/03/13 21:07:58, servolk wrote: > It might be trickier than it seems at first. If we just reverted those few > changes, but left the rest of the code intact, still how would we deal with > having to invoke the actual codec checks from net/, given it can't have > dependency on media/ ? I don't think any of the net/ uses of this code need the codecs. IsSupportedNonImageMimeType() only looks in non_image_map_, which is just container types. > That's why I suggested you try creating a patch and > testing it yourself :) While I could patch in this CL and try it out, I think it would be more effective to see it uploaded as a patch set in this CL so it can be diffed and commented on. It's just three sections of code to copy from master to your branch.
On 2015/03/13 21:16:00, Ryan Sleevi wrote: > On 2015/03/13 21:07:58, servolk wrote: > > > > This would break the dependency of net/ on mime type checking code. That would > > allow us to proceed with moving mime type checking code into content/ and > > media/, without removing any code from net::Filter for now. But Ryan was > against > > that CL. Perhaps you can help me convince him that it's a better plan than > > waiting for gathering UMA statistics (as far as I know there's no ETA for that > > from net/ team). > > We have stated repeatedly - in hangout, in email, and in IM - that this will be > a few weeks. That was an explicit ETA we provided and agreed to. > > Just please go star/read > https://code.google.com/p/chromium/issues/detail?id=318217#c9 > > I will continue to block any CLs that attempt to refactor this code to > accomplish some net/ / media/ split until the plan we all agreed to is complete. > Please don't take that as an ultimatum or an excuse to find other reviewers - > this is something the //net team *is* owning and *is* working on, and I'm > getting frustrated having to repeat it :( > > Cheers Sorry Ryan, after our discussion about GYP/GN workaround, I went and updated the HEVC CL and this concern kind of fell off my radar, so I haven't been following updates on crbug.com/318217 progress for a while. I'm glad to learn we are making progress there! Re ETA - I meant a set date, e.g. something like "we commit to have this done by Apr 2015 or Chrome M43 or ...". Saying "a few weeks" is a just little vague. I didn't mean to be snarky, sorry if my wording was off.
Talked to Ryan offline. The current plan SGTM. You can ignore my proposed workaround. Sorry for the noise.
dalecurtis@chromium.org changed reviewers: - dalecurtis@chromium.org |