Linux: make it possible to compile against libav when use_system_ffmpeg==1

media/filters/audio_file_reader.cc

Index: media/filters/audio_file_reader.cc
diff --git a/media/filters/audio_file_reader.cc b/media/filters/audio_file_reader.cc
index 32627b04b10ad558bab4b4facc386da0a3c4552d..79ca86a68157f54858fdbcd7ec21c831cee73ccf 100644
--- a/media/filters/audio_file_reader.cc
+++ b/media/filters/audio_file_reader.cc
@@ -180,12 +180,12 @@ int AudioFileReader::Read(AudioBus* audio_bus) {
       }
 
       if (av_frame->sample_rate != sample_rate_ ||
-          av_frame->channels != channels_ ||
+          av_frame_get_channels(av_frame.get()) != channels_ ||

[DaleCurtis, 2013/04/15 23:12:53]

Why? When use_system_ffmpeg is used the headers used to compile should match the
system binary; so channels access directly should be fine.

[Paweł Hajdan Jr., 2013/04/15 23:51:49]

libav's AVFrame doesn't have channels member.

[DaleCurtis, 2013/04/16 20:46:56]

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.

[Paweł Hajdan Jr., 2013/04/16 20:59:36]

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.

[DaleCurtis, 2013/04/16 21:25:56]

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.

[Paweł Hajdan Jr., 2013/04/16 21:50:35]

Note this is just for use_system_ffmpeg=1 case.
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.
In libav it's a correct assumption to make (I've asked libav developers for
advice) - in ffmpeg there are no such guarantees.

[DaleCurtis, 2013/04/17 21:17:09]

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 :)

[Paweł Hajdan Jr., 2013/04/17 22:07:34]

Done.
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.

           av_frame->format != av_sample_format_) {
         DLOG(ERROR) << "Unsupported midstream configuration change!"
                     << " Sample Rate: " << av_frame->sample_rate << " vs "
                     << sample_rate_
-                    << ", Channels: " << av_frame->channels << " vs "
+                    << ", Channels: " << av_frame_get_channels(av_frame.get()) << " vs "
                     << channels_
                     << ", Sample Format: " << av_frame->format << " vs "
                     << av_sample_format_;