ffmpeg: reallocate output buffer dynamically #746

equeim · 2023-10-19T21:12:54Z

With FFmpeg we can't determine size of output buffer ahead of time for all codecs, so we need to reallocate it when needed instead of simply failing.

tonihei · 2023-10-20T09:07:48Z

Thanks for the PR! I added a few comments for improvement ideas.

equeim · 2023-10-20T14:19:06Z

Thanks for the PR! I added a few comments for improvement ideas.

Where? 🤔 @tonihei

tonihei · 2023-10-20T08:48:49Z

libraries/decoder_ffmpeg/src/main/jni/ffmpeg_jni.cc

+    LOGE("JNI_OnLoad: GetEnv failed");
+    return -1;
+  }
+  jclass clazz = env->FindClass("androidx/media3/decoder/ffmpeg/FfmpegAudioDecoder");


If you reference classes and methods by name from native code, you'll also need to add them to the proguard-rules.txtfile of the ffmpeg module to prevent them from being obfuscated during the build process.

tonihei · 2023-10-20T08:51:29Z

libraries/decoder/src/main/java/androidx/media3/decoder/SimpleDecoderOutputBuffer.java

@@ -49,6 +50,18 @@ public ByteBuffer init(long timeUs, int size) {
    return data;
  }

+  public ByteBuffer grow(int newSize) {


Can you add Javadoc to this method in the style of other existing Javadoc?

tonihei · 2023-10-20T08:58:15Z

libraries/decoder_ffmpeg/src/main/jni/ffmpeg_jni.cc

    int outSampleSize = av_get_bytes_per_sample(context->request_sample_fmt);
    int outSamples = swr_get_out_samples(resampleContext, sampleCount);
    int bufferOutSize = outSampleSize * channelCount * outSamples;
    if (outSize + bufferOutSize > outputSize) {
-      LOGE("Output buffer size (%d) too small for output data (%d).",
+      LOGE("Output buffer size (%d) too small for output data (%d), reallocating buffer.",


This doesn't need to be LOGE anymore if the case is handled gracefully, we could change it to LOGD

tonihei · 2023-10-20T08:58:51Z

libraries/decoder_ffmpeg/src/main/jni/ffmpeg_jni.cc

+      outputSize = outSize + bufferOutSize;
+      outputBuffer = growBuffer(outputSize);
+      if (outputBuffer) {
+        LOGE("Reallocated output buffer.");


No need for this log line either

tonihei · 2023-10-20T09:01:30Z

libraries/decoder_ffmpeg/src/main/java/androidx/media3/decoder/ffmpeg/FfmpegAudioDecoder.java

-    ByteBuffer outputData = outputBuffer.init(inputBuffer.timeUs, outputBufferSize);
-    int result = ffmpegDecode(nativeContext, inputData, inputSize, outputData, outputBufferSize);
+    outputBuffer.init(inputBuffer.timeUs, outputBufferSize);
+    Assertions.checkNotNull(outputBuffer.data);


You can leave out this check as this is guaranteed by the API contract of the init method.

Ok, but then IDE paints outputBuffer.data on the next line yellow.

tonihei · 2023-10-20T09:07:15Z

libraries/decoder_ffmpeg/src/main/java/androidx/media3/decoder/ffmpeg/FfmpegAudioDecoder.java

    return null;
  }

+  // Called from native code
+  @Keep


We don't use this annotation anywhere else at the moment. I'm not sure, but I think the proguard rules I asked about in another comment should be sufficient to keep the method.

tonihei · 2023-10-30T09:32:18Z

Where? 🤔 @tonihei

Oops, sorry, I forgot to click on "publish review"!

tonihei · 2023-10-31T11:55:56Z

Thanks! I'll now import this PR internally and you may see some further clean-up commits appear here.

With FFmpeg we can't determine size of output buffer ahead of time for all codecs, so we need to reallocate it when needed instead of simply failing.

Also amended Javadoc and added assertion to grow method

Tolriq · 2023-11-01T18:02:39Z

@tonihei Your commit 8750ed8 breaks this.

By using a variable to hold the buffer you keep a reference to the old buffer at the end of the function.

So that call outputData.limit(result); fails as the result value can be related to the new buffer size but outputData is still the lowersize buffer and you have Caused by: java.lang.IllegalArgumentException: newLimit > capacity: (73728 > 65535) errors.

tonihei · 2023-11-02T08:48:04Z

Oh yes, sorry, that was clearly too much "clean-up" :) Will send a fix commit straight-away.

tonihei · 2023-11-02T10:58:02Z

Should be fixed by ae6f83d

Tolriq · 2023-11-02T15:16:23Z

@tonihei yes the fix works but the keep rules are not working :(
We need to also keep SimpleDecoderOutputBuffer else it's renamed and eventually merged and the GetMethodID fails.

tonihei · 2023-11-02T15:35:01Z

Do you want to send another PR given you can actually test this out? I was working by looking on the code only without an actual test case.

Tolriq · 2023-11-02T15:42:59Z

@tonihei In this case just compile any app with ffmpeg with R8 and the extension will fail to load in the JNI_OnLoad so no need for a file to repro.

And I can send you a file that repro this by mail if you want for future reference too.

For the PR even after all those years I'm still far from a proguard/R8 expert.

So -keep class androidx.media3.decoder.SimpleDecoderOutputBuffer { *; } Do work and fix this, but there's probably a more targeted solution to this.

As bonus questions since there's movement in that extension: Is there any chance that #707 be considered? And if yes is there any interest in a PR that adds resampling to ffmpeg extension so that hi res media that Android can't play can properly be downsampled to the max supported sample rate ?

tonihei · 2023-11-02T15:48:20Z

Yes, if you could send a file that needs this logic to android-media-github@google.com with "Issue #746" in the subject, that would be great so that I can just test it myself.

Tolriq · 2023-11-03T09:36:44Z

And just in case I was not clear enough @tonihei ffmpeg extension in main is broken for any file in current state if you build with R8 as it fails to load the extension at all.

tonihei · 2023-11-03T11:36:35Z

@Tolriq I can't reproduce the R8 failure I'm afraid. I'm testing on the latest main branch demo app in release mode with your example file. And everything works as expected (including the automated buffer resizing).

The existing proguard rule seems to ensure that the SimpleDecoderOutputBuffer class is kept already. I added

-whyareyoukeeping class androidx.media3.decoder.SimpleDecoderOutputBuffer

to get an explanation for why this class is kept in my build and the output directly references the existing rule:

androidx.media3.decoder.SimpleDecoderOutputBuffer
|- is referenced from:
|  java.nio.ByteBuffer androidx.media3.decoder.ffmpeg.FfmpegAudioDecoder.growOutputBuffer(androidx.media3.decoder.SimpleDecoderOutputBuffer,int)
|- is referenced in keep rule:
|  [...]/media/libraries/decoder_ffmpeg/buildout/intermediates/consumer_proguard_dir/release/lib0/proguard.txt:9:1

Could you check that the Ffmpeg proguard rules are included in your build? (maybe you need to clean caches/builds and try again?)

Tolriq · 2023-11-03T11:44:58Z

@tonihei The rules are included I'm using R8 with full mode and version "8.2.34"

In that mode the SimpleDecoderOutputBuffer is renamed and merged leading to the issue.

Maybe it's an R8 issue and the fact that the class is passed as a param should ensure it's kept, but from my understanding of R8/Proguard there's no guarantee about that with the current rule.

The rule

-keep class androidx.media3.decoder.ffmpeg.FfmpegAudioDecoder {
  private java.nio.ByteBuffer growOutputBuffer(androidx.media3.decoder.SimpleDecoderOutputBuffer, int);
}

Just implies that the class and function is kept nothing about the parameters.

And in the code we do reflection with

  growOutputBufferMethod =
      env->GetMethodID(clazz, "growOutputBuffer",
                       "(Landroidx/media3/decoder/"
                       "SimpleDecoderOutputBuffer;I)Ljava/nio/ByteBuffer;");

That explicitly require the type and name to be kept.

I can open a question on R8 team about this, and come back here if you want their confirmation.

Edit: Opened https://issuetracker.google.com/issues/309068090 they are very efficient so we'll have an answer soon.

tonihei · 2023-11-03T12:02:38Z

Let me try the full mode too (that wasn't enabled in my test I believe)

tonihei · 2023-11-03T13:33:57Z

Thanks for following up on the issue with R8. I still can't reproduce it in any way though. I upgraded to R8 8.2.33 (as part of the Android Studio Gradle plugin 8.2.0-rc02), but can't see the issue with or without full mode. Not sure where the difference is to be honest.

Could I ask you to test if includedescriptorclasses works in your setup? If yes, I'll submit this change to fix this.

Tolriq · 2023-11-03T14:11:30Z

@tonihei yes it works with:

# This method is called from native code
-keep, includedescriptorclasses class androidx.media3.decoder.ffmpeg.FfmpegAudioDecoder {
  private java.nio.ByteBuffer growOutputBuffer(androidx.media3.decoder.SimpleDecoderOutputBuffer, int);
}

tonihei · 2023-11-03T14:12:51Z

Great, will send a fix for this then! Thanks for your help :)

Tolriq · 2023-11-03T14:15:00Z

May I ask again some help on #369 then ? ;) Kills me to no be able to provide the test file to get this merged.

tonihei · 2023-11-06T09:15:26Z

May I ask again some help on #369 then ?

I think this fell through the cracks a bit because we were waiting to see if there is a way to produce a test file for unit tests.

tonihei self-assigned this Oct 20, 2023

tonihei reviewed Oct 30, 2023

View reviewed changes

equeim force-pushed the ffmpeg-output-buffer-reallocation branch from fe4bd76 to a66683a Compare October 30, 2023 16:23

tonihei added the should merge label Oct 31, 2023

equeim and others added 2 commits October 31, 2023 12:25

ffmpeg: reallocate output buffer dynamically

adee462

With FFmpeg we can't determine size of output buffer ahead of time for all codecs, so we need to reallocate it when needed instead of simply failing.

Format with google-java-format

8750ed8

Also amended Javadoc and added assertion to grow method

tonihei force-pushed the ffmpeg-output-buffer-reallocation branch from a66683a to 8750ed8 Compare October 31, 2023 12:28

tonihei added 4 commits October 31, 2023 12:29

Change warning suppression style

36fb207

Fix typo

78142d3

Add terminating newline

c4c4ef9

Satisfy nullness checker correctness by using local variable

58d8850

copybara-service bot merged commit 4f99000 into androidx:main Oct 31, 2023
1 check passed

androidx locked and limited conversation to collaborators Dec 31, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ffmpeg: reallocate output buffer dynamically #746

ffmpeg: reallocate output buffer dynamically #746

equeim commented Oct 19, 2023

tonihei commented Oct 20, 2023

equeim commented Oct 20, 2023 •

edited

tonihei Oct 20, 2023

equeim Oct 30, 2023

tonihei Oct 20, 2023

equeim Oct 30, 2023

tonihei Oct 20, 2023

equeim Oct 30, 2023

tonihei Oct 20, 2023

equeim Oct 30, 2023

tonihei Oct 20, 2023

equeim Oct 30, 2023

tonihei Oct 20, 2023

equeim Oct 30, 2023

tonihei commented Oct 30, 2023

tonihei commented Oct 31, 2023

Tolriq commented Nov 1, 2023

tonihei commented Nov 2, 2023

tonihei commented Nov 2, 2023

Tolriq commented Nov 2, 2023

tonihei commented Nov 2, 2023

Tolriq commented Nov 2, 2023

tonihei commented Nov 2, 2023

Tolriq commented Nov 3, 2023

tonihei commented Nov 3, 2023

Tolriq commented Nov 3, 2023 •

edited

tonihei commented Nov 3, 2023

tonihei commented Nov 3, 2023

Tolriq commented Nov 3, 2023

tonihei commented Nov 3, 2023

Tolriq commented Nov 3, 2023

tonihei commented Nov 6, 2023

ffmpeg: reallocate output buffer dynamically #746

ffmpeg: reallocate output buffer dynamically #746

Conversation

equeim commented Oct 19, 2023

tonihei commented Oct 20, 2023

equeim commented Oct 20, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tonihei commented Oct 30, 2023

tonihei commented Oct 31, 2023

Tolriq commented Nov 1, 2023

tonihei commented Nov 2, 2023

tonihei commented Nov 2, 2023

Tolriq commented Nov 2, 2023

tonihei commented Nov 2, 2023

Tolriq commented Nov 2, 2023

tonihei commented Nov 2, 2023

Tolriq commented Nov 3, 2023

tonihei commented Nov 3, 2023

Tolriq commented Nov 3, 2023 • edited

tonihei commented Nov 3, 2023

tonihei commented Nov 3, 2023

Tolriq commented Nov 3, 2023

tonihei commented Nov 3, 2023

Tolriq commented Nov 3, 2023

tonihei commented Nov 6, 2023

equeim commented Oct 20, 2023 •

edited

Tolriq commented Nov 3, 2023 •

edited