usage of ffmpeg internals in media/filters/ffmpeg_demuxer.cc

[Paweł Hajdan, Jr.]

I was experimenting with using system ffmpeg (based on https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=763632#29) and found only small changes are needed.

This could be of general interest, because the changes are related to using a private ffmpeg APIs - i.e. it doesn't seem they were intended to be exposed and used outside of ffmpeg itself.

Do you think it'd be worth it to try to work with ffmpeg upstream to figure out how to make this a public API instead that Chrome could use?

This is how the change to ffmpeg_demuxer.cc looks like:

diff --git a/media/filters/ffmpeg_demuxer.cc b/media/filters/ffmpeg_demuxer.cc

index 155e980..7ba327a 100644

--- a/media/filters/ffmpeg_demuxer.cc

+++ b/media/filters/ffmpeg_demuxer.cc

@@ -966,24 +966,6 @@ void FFmpegDemuxer::OnFindStreamInfoDone(const PipelineStatusCB& status_cb,

   // If no estimate is found, the stream entry will be kInfiniteDuration().

   std::vector<base::TimeDelta> start_time_estimates(format_context->nb_streams,

                                                     kInfiniteDuration());

-  const AVFormatInternal* internal = format_context->internal;

-  if (internal && internal->packet_buffer &&

-      format_context->start_time != static_cast<int64>(AV_NOPTS_VALUE)) {

-    struct AVPacketList* packet_buffer = internal->packet_buffer;

-    while (packet_buffer != internal->packet_buffer_end) {

-      DCHECK_LT(static_cast<size_t>(packet_buffer->pkt.stream_index),

-                start_time_estimates.size());

-      const AVStream* stream =

-          format_context->streams[packet_buffer->pkt.stream_index];

-      if (packet_buffer->pkt.pts != static_cast<int64>(AV_NOPTS_VALUE)) {

-        const base::TimeDelta packet_pts =

-            ConvertFromTimeBase(stream->time_base, packet_buffer->pkt.pts);

-        if (packet_pts < start_time_estimates[stream->index])

-          start_time_estimates[stream->index] = packet_pts;

-      }

-      packet_buffer = packet_buffer->next;

-    }

-  }

 

   AVStream* audio_stream = NULL;

   AudioDecoderConfig audio_config;

When I apply above change to my chromium checkout in Ubuntu Trusty, media_unittests still pass. This suggests it either doesn't have proper test coverage, or above code might be unneeded.

FWIW when actually linked against system ffmpeg 2.8 (not that easy on Ubuntu Trusty, I was using a Gentoo Linux chroot for this), the following tests fail:

[ RUN      ] PipelineIntegrationTest.BasicPlaybackOpusWebmTrimmingHashed_MediaSource

../../media/test/pipeline_integration_test.cc:877: Failure

Value of: GetAudioHash()

  Actual: "-6.73,-4.16,-0.90,-0.41,-2.76,-5.68,"

Expected: kOpusEndTrimmingHash_1

Which is: "-4.56,-5.65,-6.51,-6.29,-4.36,-3.59,"

../../media/test/pipeline_integration_test.cc:885: Failure

Value of: GetAudioHash()

  Actual: "-5.64,-4.37,-3.87,-4.65,-5.96,-6.76,"

Expected: kOpusEndTrimmingHash_2

Which is: "-11.89,-11.09,-8.25,-7.11,-7.84,-9.97,"

../../media/test/pipeline_integration_test.cc:894: Failure

Value of: GetAudioHash()

  Actual: "-8.54,-8.24,-8.01,-8.37,-8.03,-8.00,"

Expected: kOpusEndTrimmingHash_3

Which is: "-13.28,-14.35,-13.67,-11.68,-10.18,-10.46,"

[  FAILED  ] PipelineIntegrationTest.BasicPlaybackOpusWebmTrimmingHashed_MediaSource (115 ms)

[2412/2414] PipelineIntegrationTest.BasicPlaybackOpusWebmTrimmingHashed_MediaSource (115 ms)

[ RUN      ] PipelineIntegrationTest.BasicPlaybackOpusWebmTrimmingHashed

../../media/test/pipeline_integration_test.cc:851: Failure

Value of: GetAudioHash()

  Actual: "-6.73,-4.16,-0.90,-0.41,-2.76,-5.68,"

Expected: kOpusEndTrimmingHash_1

Which is: "-4.56,-5.65,-6.51,-6.29,-4.36,-3.59,"

../../media/test/pipeline_integration_test.cc:857: Failure

Value of: GetAudioHash()

  Actual: "-5.64,-4.37,-3.87,-4.65,-5.96,-6.76,"

Expected: kOpusEndTrimmingHash_2

Which is: "-11.89,-11.09,-8.25,-7.11,-7.84,-9.97,"

../../media/test/pipeline_integration_test.cc:864: Failure

Value of: GetAudioHash()

  Actual: "-8.54,-8.24,-8.01,-8.37,-8.03,-8.00,"

Expected: kOpusEndTrimmingHash_3

Which is: "-13.28,-14.35,-13.67,-11.68,-10.18,-10.46,"

[  FAILED  ] PipelineIntegrationTest.BasicPlaybackOpusWebmTrimmingHashed (129 ms)

[2413/2414] PipelineIntegrationTest.BasicPlaybackOpusWebmTrimmingHashed (129 ms)

[ RUN      ] PipelineIntegrationTest.BasicPlaybackOpusOggTrimmingHashed

../../media/test/pipeline_integration_test.cc:828: Failure

Value of: GetAudioHash()

  Actual: "-6.73,-4.16,-0.90,-0.41,-2.76,-5.68,"

Expected: kOpusEndTrimmingHash_1

Which is: "-4.56,-5.65,-6.51,-6.29,-4.36,-3.59,"

../../media/test/pipeline_integration_test.cc:834: Failure

Value of: GetAudioHash()

  Actual: "-5.64,-4.37,-3.87,-4.65,-5.96,-6.76,"

Expected: kOpusEndTrimmingHash_2

Which is: "-11.89,-11.09,-8.25,-7.11,-7.84,-9.97,"

../../media/test/pipeline_integration_test.cc:841: Failure

Value of: GetAudioHash()

  Actual: "-8.54,-8.24,-8.01,-8.37,-8.03,-8.00,"

Expected: kOpusEndTrimmingHash_3

Which is: "-13.28,-14.35,-13.67,-11.68,-10.18,-10.46,"

[  FAILED  ] PipelineIntegrationTest.BasicPlaybackOpusOggTrimmingHashed (127 ms)

[2414/2414] PipelineIntegrationTest.BasicPlaybackOpusOggTrimmingHashed (127 ms)

3 tests failed:

    PipelineIntegrationTest.BasicPlaybackOpusOggTrimmingHashed (../../media/test/pipeline_integration_test.cc:821)

    PipelineIntegrationTest.BasicPlaybackOpusWebmTrimmingHashed (../../media/test/pipeline_integration_test.cc:844)

    PipelineIntegrationTest.BasicPlaybackOpusWebmTrimmingHashed_MediaSource (../../media/test/pipeline_integration_test.cc:868)

For completeness, I've needed one more change to make things work with system ffmpeg 2.8 (note how the #defines being removed affect the ABI - otherwise the code would crash when linked against system ffmpeg):

diff --git a/media/ffmpeg/ffmpeg_common.h b/media/ffmpeg/ffmpeg_common.h

index 677bd76..23a21f8 100644

--- a/media/ffmpeg/ffmpeg_common.h

+++ b/media/ffmpeg/ffmpeg_common.h

@@ -19,20 +19,12 @@

 

 // Include FFmpeg header files.

 extern "C" {

-// Disable deprecated features which result in spammy compile warnings.  This

-// list of defines must mirror those in the 'defines' section of the ffmpeg.gyp

-// file or the headers below will generate different structures.

-#define FF_API_PIX_FMT_DESC 0

-#define FF_API_OLD_DECODE_AUDIO 0

-#define FF_API_DESTRUCT_PACKET 0

-#define FF_API_GET_BUFFER 0

 

 // Temporarily disable possible loss of data warning.

 // TODO(scherkus): fix and upstream the compiler warnings.

 MSVC_PUSH_DISABLE_WARNING(4244);

 #include <libavcodec/avcodec.h>

 #include <libavformat/avformat.h>

-#include <libavformat/internal.h>

 #include <libavformat/avio.h>

 #include <libavutil/avutil.h>

 #include <libavutil/imgutils.h>

Pawel

[Dale Curtis]

The first change is only there because ffmpeg doesn't reliably expose the start_time for a stream. We have to hunt through the internal packet lists for it. Fixing that bug would allow us to remove the internal code.

The second change would actually be a bit harder, as while we can disable the deprecated functions warning in ffmpeg easily, I think we'd now need to modify all transitive use within media/.

Either way I'm generally supportive of clean fixes to this problem that provide external users something useful. Last we heard it seemed most distros just want ffmpeg as a shared library and some third party site provides a drop in for Chromium?

Even that I don't really the utility of though, since the way we hardcode supported codecs seems to eliminate the value of a drop in. Chrome also updates ffmpeg more frequently than others as far as I've seen, so whatever we're using has all the latest security fixes.

- dale

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev

[Paweł Hajdan, Jr.]

On Mon, Sep 21, 2015 at 6:34 PM, Dale Curtis <dalec...@chromium.org> wrote:

The first change is only there because ffmpeg doesn't reliably expose the start_time for a stream. We have to hunt through the internal packet lists for it. Fixing that bug would allow us to remove the internal code.

Okay. Is there a bug on file for this - either on chromium side, or ideally, ffmpeg?

FWIW I realized that tests do fail when the change is made - https://codereview.chromium.org/1359653003 .

 

The second change would actually be a bit harder, as while we can disable the deprecated functions warning in ffmpeg easily, I think we'd now need to modify all transitive use within media/.

Either way I'm generally supportive of clean fixes to this problem that provide external users something useful. Last we heard it seemed most distros just want ffmpeg as a shared library and some third party site provides a drop in for Chromium?

Even that I don't really the utility of though, since the way we hardcode supported codecs seems to eliminate the value of a drop in. Chrome also updates ffmpeg more frequently than others as far as I've seen, so whatever we're using has all the latest security fixes.

Makes sense.

I'm not advocating for applying the second hunk to chromium repo. Just mentioned it for the context.

For codec support, do you have recommendations for testing e.g. h264 and mp3 support? Are there any other codec differences between Chromium and Chrome-branded ffmpeg?

Paweł

[Dale Curtis]

On Mon, Sep 21, 2015 at 1:11 PM, Paweł Hajdan, Jr. <phajd...@chromium.org> wrote:

On Mon, Sep 21, 2015 at 6:34 PM, Dale Curtis <dalec...@chromium.org> wrote:

The first change is only there because ffmpeg doesn't reliably expose the start_time for a stream. We have to hunt through the internal packet lists for it. Fixing that bug would allow us to remove the internal code.

Okay. Is there a bug on file for this - either on chromium side, or ideally, ffmpeg?

Just a Chrome side bug which tracked why we added this code, http://code.google.com/p/chromium/issues/detail?id=377295

 

FWIW I realized that tests do fail when the change is made - https://codereview.chromium.org/1359653003 .

 

The second change would actually be a bit harder, as while we can disable the deprecated functions warning in ffmpeg easily, I think we'd now need to modify all transitive use within media/.

Either way I'm generally supportive of clean fixes to this problem that provide external users something useful. Last we heard it seemed most distros just want ffmpeg as a shared library and some third party site provides a drop in for Chromium?

Even that I don't really the utility of though, since the way we hardcode supported codecs seems to eliminate the value of a drop in. Chrome also updates ffmpeg more frequently than others as far as I've seen, so whatever we're using has all the latest security fixes.

Makes sense.

I'm not advocating for applying the second hunk to chromium repo. Just mentioned it for the context.

For codec support, do you have recommendations for testing e.g. h264 and mp3 support? Are there any other codec differences between Chromium and Chrome-branded ffmpeg?

Depends on what you mean by testing. We have tests already in Chrome which verify expected behavior as we define it. You can find the full list of codecs here:

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/ffmpeg/chromium/scripts/build_ffmpeg.py&l=494

[Paweł Hajdan, Jr.]

On Mon, Sep 21, 2015 at 10:44 PM, Dale Curtis <dalec...@chromium.org> wrote:

On Mon, Sep 21, 2015 at 1:11 PM, Paweł Hajdan, Jr. <phajd...@chromium.org> wrote:

On Mon, Sep 21, 2015 at 6:34 PM, Dale Curtis <dalec...@chromium.org> wrote:

The first change is only there because ffmpeg doesn't reliably expose the start_time for a stream. We have to hunt through the internal packet lists for it. Fixing that bug would allow us to remove the internal code.

Okay. Is there a bug on file for this - either on chromium side, or ideally, ffmpeg?

Just a Chrome side bug which tracked why we added this code, http://code.google.com/p/chromium/issues/detail?id=377295

Right. How about a bug so that a public ffmpeg API can be used as opposed to the internals?

For codec support, do you have recommendations for testing e.g. h264 and mp3 support? Are there any other codec differences between Chromium and Chrome-branded ffmpeg?

Depends on what you mean by testing. We have tests already in Chrome which verify expected behavior as we define it. You can find the full list of codecs here:

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/ffmpeg/chromium/scripts/build_ffmpeg.py&l=494

Thanks!

Are there some specific sample files I could manually test a custom Chromium build with?

h264, aac, and mp3 seem relatively straightforward, although I wouldn't mind if there was e.g. a simple page to test all of them with known-good samples.

I'm more confused by e.g. mov and mpegaudio - how can I get some test files for these?

What is the reason for mov only being enabled for Chrome branding and not Chromium?

Also (just curiosity), what's the difference between a decoder, demuxer, and parser? I read e.g. http://stackoverflow.com/questions/9956755/concept-about-multimedia-codecs-container-format-codec-muxer-demuxer but some things seem still unclear. For example, why do aac and mp3 appear as both decoder and demuxer? And what exactly is a parser, and how is it different from other two? How are they all related to a container?

Paweł

[Dale Curtis]

Argh, from the right account this time.

On Tue, Sep 22, 2015 at 6:16 AM, Paweł Hajdan, Jr. <phajd...@chromium.org> wrote:

On Mon, Sep 21, 2015 at 10:44 PM, Dale Curtis <dalec...@chromium.org> wrote:

On Mon, Sep 21, 2015 at 1:11 PM, Paweł Hajdan, Jr. <phajd...@chromium.org> wrote:

On Mon, Sep 21, 2015 at 6:34 PM, Dale Curtis <dalec...@chromium.org> wrote:

The first change is only there because ffmpeg doesn't reliably expose the start_time for a stream. We have to hunt through the internal packet lists for it. Fixing that bug would allow us to remove the internal code.

Okay. Is there a bug on file for this - either on chromium side, or ideally, ffmpeg?

Just a Chrome side bug which tracked why we added this code, http://code.google.com/p/chromium/issues/detail?id=377295

Right. How about a bug so that a public ffmpeg API can be used as opposed to the internals?

For codec support, do you have recommendations for testing e.g. h264 and mp3 support? Are there any other codec differences between Chromium and Chrome-branded ffmpeg?

Depends on what you mean by testing. We have tests already in Chrome which verify expected behavior as we define it. You can find the full list of codecs here:

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/ffmpeg/chromium/scripts/build_ffmpeg.py&l=494

Thanks!

Are there some specific sample files I could manually test a custom Chromium build with?

media/test/data has all the files we use for tests. We have an internal matrix which I've sent you the link separately for.

 

h264, aac, and mp3 seem relatively straightforward, although I wouldn't mind if there was e.g. a simple page to test all of them with known-good samples.

I'm more confused by e.g. mov and mpegaudio - how can I get some test files for these? 

What is the reason for mov only being enabled for Chrome branding and not Chromium?

mov == mp4, we don't support any of the Chromium codecs in mp4, thus the demuxer is not compiled in.

 

Also (just curiosity), what's the difference between a decoder, demuxer, and parser? I read e.g. http://stackoverflow.com/questions/9956755/concept-about-multimedia-codecs-container-format-codec-muxer-demuxer but some things seem still unclear. For example, why do aac and mp3 appear as both decoder and demuxer? And what exactly is a parser, and how is it different from other two? How are they all related to a container?

Demuxers separate encoded packets out of a bytestream. Parsers help demuxers in cases where codec specific data is necessary to parcel out encoded packets. Decoders turn encoded packets into raw audio or video frames.

Both mp3 and aac can be stored as raw encoded packets w/o the need of a higher order container like mp4, webm, etc. When stored like this, ffmpeg uses the mp3/aac demuxers. http://www.w3.org/TR/media-source/mpeg-audio-byte-stream-format.html has some more info on this particular case.

- dale

[Allan Sandfeld Jensen]

On Monday 21 September 2015, Paweł Hajdan, Jr. wrote:
> I was experimenting with using system ffmpeg (based on
> https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=763632#29) and found only
> small changes are needed.
>
> This could be of general interest, because the changes are related to using
> a private ffmpeg APIs - i.e. it doesn't seem they were intended to be
> exposed and used outside of ffmpeg itself.
>
> Do you think it'd be worth it to try to work with ffmpeg upstream to figure
> out how to make this a public API instead that Chrome could use?
>

We use a very similar change in QtWebEngine, but only when building with
system FFMPEG. See https://codereview.qt-project.org/#/c/121170/2//ALL,unified

Best regards
`Allan

[Paweł Hajdan, Jr.]

I'm wondering what would it take to make changes to ffmpeg upstream to make such custom patches unnecessary. I'm referring specifically to the "const AVFormatInternal* internal = format_context->internal" part.

Dale, WDYT?

Paweł

[Dale Curtis]

I think that's a good idea.

[Paweł Hajdan, Jr.]

I'm glad we agree on that. :)

Now I was looking for some guidance. It seems to me we'd need to add some kind of public API to ffmpeg to allow Chromium media stack to do what the referenced piece of code currently does without need to reach into internals.

I was just wondering if you had some hints how such an API would look like to suite media needs best. I could reasonably produce some patches to make things work in terms of code, but I'm much less confident that the result would be a good API.

I was also looking for your advice about how to suggest changes to the ffmpeg community. Would you e.g. recommend starting a thread similar to this on ffmpeg-devel, or should I rather start with having some patch ready for more specific discussion?

Paweł

[Dale Curtis]

There's already a public API, it just needs to be fixed; see the start_time field on the AVFormatContext / AVStream structures. Unfortunately it's not always accurate or is missing without the additional helper code. It's possible that just showing upstream what we've added plus the clips involved would allow them to root out the problem.

- dale