Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
13991013:
Linux: make it possible to compile against libav when use_system_ffmpeg==1 (Closed)
|
Created:
7 years, 8 months ago by Paweł Hajdan Jr. Modified:
7 years, 8 months ago Reviewers:
DaleCurtis CC:
chromium-reviews, feature-media-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionLinux: make it possible to compile against libav when use_system_ffmpeg==1
libav's AVFrame doesn't have channels field.
BUG=none
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=195001
Patch Set 1 #Patch Set 2 : tabs #
Total comments: 12
Patch Set 3 : #ifdefs #
Total comments: 4
Patch Set 4 : varz #
Messages
Total messages: 16 (0 generated)
https://codereview.chromium.org/13991013/diff/2001/media/filters/audio_file_r... File media/filters/audio_file_reader.cc (right): https://codereview.chromium.org/13991013/diff/2001/media/filters/audio_file_r... media/filters/audio_file_reader.cc:183: av_frame_get_channels(av_frame.get()) != channels_ || Why? When use_system_ffmpeg is used the headers used to compile should match the system binary; so channels access directly should be fine. https://codereview.chromium.org/13991013/diff/2001/media/media.gyp File media/media.gyp (right): https://codereview.chromium.org/13991013/diff/2001/media/media.gyp#newcode511 media/media.gyp:511: 'cflags': [ This section is pretty different than the previous. I don't understand your changes here. Has compile_test changed in some way?
https://codereview.chromium.org/13991013/diff/2001/media/filters/audio_file_r... File media/filters/audio_file_reader.cc (right): https://codereview.chromium.org/13991013/diff/2001/media/filters/audio_file_r... media/filters/audio_file_reader.cc:183: av_frame_get_channels(av_frame.get()) != channels_ || On 2013/04/15 23:12:53, DaleCurtis wrote: > Why? When use_system_ffmpeg is used the headers used to compile should match the > system binary; so channels access directly should be fine. libav's AVFrame doesn't have channels member. https://codereview.chromium.org/13991013/diff/2001/media/media.gyp File media/media.gyp (right): https://codereview.chromium.org/13991013/diff/2001/media/media.gyp#newcode511 media/media.gyp:511: 'cflags': [ On 2013/04/15 23:12:53, DaleCurtis wrote: > This section is pretty different than the previous. I don't understand your > changes here. Has compile_test changed in some way? defines -> cflags avoids a problem with gyp appending empty -D switches (may be a change in gyp, may be a result of my test with more recent libraries) __STDC_CONSTANT_MACROS is to avoid a spurious failure that happens when using C++ compiler and makes the compile test always fail.
https://codereview.chromium.org/13991013/diff/2001/media/filters/audio_file_r... File media/filters/audio_file_reader.cc (right): https://codereview.chromium.org/13991013/diff/2001/media/filters/audio_file_r... media/filters/audio_file_reader.cc:183: av_frame_get_channels(av_frame.get()) != channels_ || On 2013/04/15 23:51:49, Paweł Hajdan Jr. wrote: > On 2013/04/15 23:12:53, DaleCurtis wrote: > > Why? When use_system_ffmpeg is used the headers used to compile should match > the > > system binary; so channels access directly should be fine. > > libav's AVFrame doesn't have channels member. It doesn't appear to have an av_frame_get_channels() method either? I only see av_get_channel_layout_nb_channels(frame->channel_layout); In the libav code. https://codereview.chromium.org/13991013/diff/2001/media/media.gyp File media/media.gyp (right): https://codereview.chromium.org/13991013/diff/2001/media/media.gyp#newcode511 media/media.gyp:511: 'cflags': [ On 2013/04/15 23:51:49, Paweł Hajdan Jr. wrote: > On 2013/04/15 23:12:53, DaleCurtis wrote: > > This section is pretty different than the previous. I don't understand your > > changes here. Has compile_test changed in some way? > > defines -> cflags avoids a problem with gyp appending empty -D switches (may be > a change in gyp, may be a result of my test with more recent libraries) > > __STDC_CONSTANT_MACROS is to avoid a spurious failure that happens when using > C++ compiler and makes the compile test always fail. Can you put the #define and #include in a variable?
https://codereview.chromium.org/13991013/diff/2001/media/filters/audio_file_r... File media/filters/audio_file_reader.cc (right): https://codereview.chromium.org/13991013/diff/2001/media/filters/audio_file_r... media/filters/audio_file_reader.cc:183: av_frame_get_channels(av_frame.get()) != channels_ || On 2013/04/16 20:46:56, DaleCurtis wrote: > On 2013/04/15 23:51:49, Paweł Hajdan Jr. wrote: > > On 2013/04/15 23:12:53, DaleCurtis wrote: > > > Why? When use_system_ffmpeg is used the headers used to compile should match > > the > > > system binary; so channels access directly should be fine. > > > > libav's AVFrame doesn't have channels member. > > It doesn't appear to have an av_frame_get_channels() method either? I only see > > av_get_channel_layout_nb_channels(frame->channel_layout); > > In the libav code. Correct. I'm adding code that probes for presence of av_frame_get_channels. If that function is detected, nothing is done. If it's not present, a macro is defined that replaces it with av_get_channel_layout_nb_channels usage. https://codereview.chromium.org/13991013/diff/2001/media/media.gyp File media/media.gyp (right): https://codereview.chromium.org/13991013/diff/2001/media/media.gyp#newcode511 media/media.gyp:511: 'cflags': [ On 2013/04/16 20:46:56, DaleCurtis wrote: > Can you put the #define and #include in a variable? I could technically, but unless you insist I'd prefer the current way. The tests are supposed to be independent - any similarities between them are accidental. It is arguably easier to reason about them when you can see the whole program. Otherwise - why not de-duplicate int main() { } in the same way? But IMO it would actually decrease readability for no gain.
https://codereview.chromium.org/13991013/diff/2001/media/filters/audio_file_r... File media/filters/audio_file_reader.cc (right): https://codereview.chromium.org/13991013/diff/2001/media/filters/audio_file_r... media/filters/audio_file_reader.cc:183: av_frame_get_channels(av_frame.get()) != channels_ || On 2013/04/16 20:59:36, Paweł Hajdan Jr. wrote: > On 2013/04/16 20:46:56, DaleCurtis wrote: > > On 2013/04/15 23:51:49, Paweł Hajdan Jr. wrote: > > > On 2013/04/15 23:12:53, DaleCurtis wrote: > > > > Why? When use_system_ffmpeg is used the headers used to compile should > match > > > the > > > > system binary; so channels access directly should be fine. > > > > > > libav's AVFrame doesn't have channels member. > > > > It doesn't appear to have an av_frame_get_channels() method either? I only see > > > > av_get_channel_layout_nb_channels(frame->channel_layout); > > > > In the libav code. > > Correct. I'm adding code that probes for presence of av_frame_get_channels. If > that function is detected, nothing is done. If it's not present, a macro is > defined that replaces it with av_get_channel_layout_nb_channels usage. Ugh, that's super gross. I didn't realize that's what you were doing. I'd prefer if you just added a #if <flag> int frame_channels = av_get_channel_layout_nb_channels(); #else int frame_channels = av_frame_->channels(); #endif Though this code was added for a security issue where channels is set incorrectly relative to channel layout. As such I don't think it will have the same effect using channel layout.
https://codereview.chromium.org/13991013/diff/2001/media/filters/audio_file_r... File media/filters/audio_file_reader.cc (right): https://codereview.chromium.org/13991013/diff/2001/media/filters/audio_file_r... media/filters/audio_file_reader.cc:183: av_frame_get_channels(av_frame.get()) != channels_ || On 2013/04/16 21:25:56, DaleCurtis wrote: > Ugh, that's super gross. I didn't realize that's what you were doing. Note this is just for use_system_ffmpeg=1 case. > I'd prefer if you just added a > > #if <flag> > int frame_channels = av_get_channel_layout_nb_channels(); > #else > int frame_channels = av_frame_->channels(); > #endif I was trying to minimize the impact on "mainline" Chromium codebase here - this solution seemed better than more #ifdefs (nobody likes #ifdefs, right?). If you really do prefer #ifdefs (which IMO will make the code harder to read), which is especially worrying because of the security fix in this very place, my suggestion for flag name is CHROMIUM_NO_AVFRAME_CHANNELS - does it sound OK? But really, seriously, I'd rather not add more #ifdefs. And then people would complain that I'm adding more of them, when it's not really necessary. > Though this code was added for a security issue where channels is set > incorrectly relative to channel layout. As such I don't think it will have the > same effect using channel layout. In libav it's a correct assumption to make (I've asked libav developers for advice) - in ffmpeg there are no such guarantees.
https://codereview.chromium.org/13991013/diff/2001/media/filters/audio_file_r... File media/filters/audio_file_reader.cc (right): https://codereview.chromium.org/13991013/diff/2001/media/filters/audio_file_r... media/filters/audio_file_reader.cc:183: av_frame_get_channels(av_frame.get()) != channels_ || On 2013/04/16 21:50:35, Paweł Hajdan Jr. wrote: > On 2013/04/16 21:25:56, DaleCurtis wrote: > > Ugh, that's super gross. I didn't realize that's what you were doing. > > Note this is just for use_system_ffmpeg=1 case. > > > I'd prefer if you just added a > > > > #if <flag> > > int frame_channels = av_get_channel_layout_nb_channels(); > > #else > > int frame_channels = av_frame_->channels(); > > #endif > > I was trying to minimize the impact on "mainline" Chromium codebase here - this > solution seemed better than more #ifdefs (nobody likes #ifdefs, right?). > > If you really do prefer #ifdefs (which IMO will make the code harder to read), > which is especially worrying because of the security fix in this very place, my > suggestion for flag name is CHROMIUM_NO_AVFRAME_CHANNELS - does it sound OK? > > But really, seriously, I'd rather not add more #ifdefs. And then people would > complain that I'm adding more of them, when it's not really necessary. > > > Though this code was added for a security issue where channels is set > > incorrectly relative to channel layout. As such I don't think it will have > the > > same effect using channel layout. > > In libav it's a correct assumption to make (I've asked libav developers for > advice) - in ffmpeg there are no such guarantees. I realize it's just for use_system and is a one off, but using -D for function replacement is really sketchy. You might as well just -Dav_frame_->channels_= av_get_channel_layout_nb_channels(av_frame_) which is equally horrible (since it rewrites the intended meaning of code silently at build time). At least with the #if you know the intended paths and can easily backtrack to see why this flag is there. It was supposed to be a correct assumption to make in FFmpeg too :)
https://codereview.chromium.org/13991013/diff/2001/media/filters/audio_file_r... File media/filters/audio_file_reader.cc (right): https://codereview.chromium.org/13991013/diff/2001/media/filters/audio_file_r... media/filters/audio_file_reader.cc:183: av_frame_get_channels(av_frame.get()) != channels_ || On 2013/04/17 21:17:09, DaleCurtis wrote: > At least with the #if you know the intended paths and can easily backtrack to > see why this flag is there. Done. > It was supposed to be a correct assumption to make in FFmpeg too :) If media_unittests+asan is expected to catch this, then I tested with system libav, and asan media_unittests didn't detect any memory problems.
It might show up if you build ffmpeg_regression_tests under asan and run them against libav. You'll need to checkout the internal security repo to do this. Ping me on chat if you're interested and I'll give you the URL.
https://codereview.chromium.org/13991013/diff/13001/media/filters/audio_file_... File media/filters/audio_file_reader.cc (right): https://codereview.chromium.org/13991013/diff/13001/media/filters/audio_file_... media/filters/audio_file_reader.cc:183: #ifdef CHROMIUM_NO_AVFRAME_CHANNELS Instead of doing this, just do one #ifdef above the block and set frame_channels as I mentioned in my initial comment. Then you don't have a double #if section. https://codereview.chromium.org/13991013/diff/13001/media/filters/ffmpeg_audi... File media/filters/ffmpeg_audio_decoder.cc (right): https://codereview.chromium.org/13991013/diff/13001/media/filters/ffmpeg_audi... media/filters/ffmpeg_audio_decoder.cc:401: #ifdef CHROMIUM_NO_AVFRAME_CHANNELS Ditto.
PTAL https://codereview.chromium.org/13991013/diff/13001/media/filters/audio_file_... File media/filters/audio_file_reader.cc (right): https://codereview.chromium.org/13991013/diff/13001/media/filters/audio_file_... media/filters/audio_file_reader.cc:183: #ifdef CHROMIUM_NO_AVFRAME_CHANNELS On 2013/04/17 22:14:23, DaleCurtis wrote: > Instead of doing this, just do one #ifdef above the block and set frame_channels > as I mentioned in my initial comment. Then you don't have a double #if section. Done. https://codereview.chromium.org/13991013/diff/13001/media/filters/ffmpeg_audi... File media/filters/ffmpeg_audio_decoder.cc (right): https://codereview.chromium.org/13991013/diff/13001/media/filters/ffmpeg_audi... media/filters/ffmpeg_audio_decoder.cc:401: #ifdef CHROMIUM_NO_AVFRAME_CHANNELS On 2013/04/17 22:14:23, DaleCurtis wrote: > Ditto. Done.
https://code.google.com/p/chromium/codesearch#chromium/src/webkit/media/crypt... Needs updating too.
On 2013/04/18 17:43:39, DaleCurtis wrote: > https://code.google.com/p/chromium/codesearch#chromium/src/webkit/media/crypt... > > Needs updating too. It didn't come up as a problem in my tests. As I have no way to say whether it works or not, and these #ifdefs wouldn't be needed, I'd rather not add them.
lgtm
Message was sent while issue was closed.
Committed patchset #4 manually as r195001 (presubmit successful). |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
