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

media/filters/audio_file_reader.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "media/filters/audio_file_reader.h"
 
 #include "base/logging.h"
 #include "base/time.h"
 #include "media/base/audio_bus.h"
 #include "media/ffmpeg/ffmpeg_common.h"
@@ skipping 162 matching lines @@
         continue;
 
       // Determine the number of sample-frames we just decoded.  Check overflow.
       int frames_read = av_frame->nb_samples;
       if (frames_read < 0) {
         continue_decoding = false;
         break;
       }
 
       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_;
 
         // This is an unrecoverable error, so bail out.
         continue_decoding = false;
         break;
       }
 
       // Truncate, if necessary, if the destination isn't big enough.
@@ skipping 31 matching lines @@
   // Zero any remaining frames.
   audio_bus->ZeroFramesPartial(
       current_frame, audio_bus->frames() - current_frame);
 
   // Returns the actual number of sample-frames decoded.
   // Ideally this represents the "true" exact length of the file.
   return current_frame;
 }
 
 }  // namespace media