patchGnash - The GNU Flash player - Patches: patch #8747, Patches to fix 8-bit PCM sound...

 
 

You are not allowed to post comments on this tracker with your current authentication level.

patch #8747: Patches to fix 8-bit PCM sound wailing/crashing in FFmpeg media backend

Submitter:  Nutchanon Wetchasit <nachanon>
Submitted:  Sun 20 Sep 2015 06:17:21 AM UTC
   
 
Category:  sound Priority:  5 - Normal
Status:  Done Privacy:  Public
Assigned to:  strk Open/Closed:  Closed

Jump to the original submission

Wed 23 Sep 2015 01:47:44 PM UTC, comment #13: 

Patches here all pushed.

Sandro Santilli <strk>
Group Member
Wed 23 Sep 2015 10:54:34 AM UTC, comment #12: 

For a corresponding code documentation fix in PCM decoder of GStreamer backend, see patch #8751.

Nutchanon Wetchasit <nachanon>
Wed 23 Sep 2015 10:28:03 AM UTC, comment #11: 

Updated series of patches:

  • 1/5: 0004_use-source-sample-format-in-ffmpeg-audio-resampler.patch (unchanged, file #34930)
  • 2/5: 0005_dont-skip-resampling-8bit-audio.patch (unchanged, file #34931)
  • 3/5: 0006_use-correct-8bit-pcm-ffmpeg-codec-v2.patch (file #34958)
  • 4/5: 0007_use-original-sample-rate-for-ffmpeg-8bit-pcm-decoder-v2.patch (file #34959)
  • 5/5: 0008-fix-ffmpeg-resample-factor-count-calculation-v2.patch (file #34960)


Note: This updated series also have been re-tested in the same configuration as comment 6.

Gnash: 0.8.11dev (patched against git 516a6ca 15-Sep-2015)
SDL: libsdl1.2debian 1.2.15-5 (debian)
libAV: libavcodec53 6:0.8.17-1 (debian), libavformat53 6:0.8.17-1 (debian), libavutil51 6:0.8.17-1 (debian)
CSound: libcsound64-5.2 1:5.17.11~dfsg-3 (debian)
System: Debian GNU/Linux 7.0 Wheezy i386

Nutchanon Wetchasit <nachanon>
Wed 23 Sep 2015 06:21:28 AM UTC, comment #10: 

Perfect, comments are ok to me. Thanks !
I'll wait for updated before committing them all.

Sandro Santilli <strk>
Group Member
Wed 23 Sep 2015 04:32:24 AM UTC, comment #9: 

Does following comments look okay (for patch 3 and patch 5)?

3/5: 0006_use-correct-8bit-pcm-ffmpeg-codec.patch:

            case AUDIO_CODEC_UNCOMPRESSED:
            case AUDIO_CODEC_RAW:
                if (info.sampleSize == 2) {
                    // Flash's 16-bit UNCOMPRESSED and RAW audio comes as
                    // a signed PCM format. UNCOMPRESSED audio always use
                    // little-endian byte order. RAW audio uses encoder's
                    // native byte order; as majority of encoders run
                    // on little-endian machine, we use little-endian for it.
                    codec_id = AV_CODEC_ID_PCM_S16LE;
                } else {
                    // Flash's 8-bit UNCOMPRESSED and RAW audio comes as an
                    // unsigned PCM format. No difference between
                    // UNCOMPRESSED and RAW.
                    codec_id = AV_CODEC_ID_PCM_U8;
                }
                break;


Note: GStreamer backend have a similar problem in its code documentation;
the code itself correctly implements a 8-bit unsigned PCM codec,
but the comments still say 8-bit signed. I'll submit a fix for that as
a different patch entry.

5/5: 0008-fix-ffmpeg-resample-factor-count-calculation.patch:

    if (_resampler.init(_audioCodecCtx)) {
        // Resampling is needed.

        // Compute new size based on frame_size and
        // resampling configuration

        // Find out the needed sample rate scaling
        double resampleFactor = 44100.0/_audioCodecCtx->sample_rate;

        // Compute total number of input samples
        int inSamples = outSize;
        bool stereo = _audioCodecCtx->channels > 1 ? true : false;

        if (stereo) inSamples = inSamples >> 1;
        if (_audioCodecCtx->sample_fmt == AV_SAMPLE_FMT_S16 ||
            _audioCodecCtx->sample_fmt == AV_SAMPLE_FMT_S16P) {
            inSamples = inSamples >> 1;
        }

        // Compute total number of output samples
        int expectedMaxOutSamples = std::ceil(inSamples*resampleFactor);

        // Compute output buffer size (in bytes); by multiplying
        // output samples count with output sample format's frame size,
        // which is number of bytes per sample (2) times channels (2).
        int resampledFrameSize = expectedMaxOutSamples*2*2;

        // Allocate just the required amount of bytes
        std::uint8_t* resampledOutput = new std::uint8_t[resampledFrameSize];


For patch 1, 2, 4; they're just fixing coding oversight and removing hacks,
I didn't really have anything else to say about them.

If comments above are OK, I'll proceed to submit new version of patch 3, 4, 5.

Nutchanon Wetchasit <nachanon>
Mon 21 Sep 2015 08:13:05 AM UTC, comment #8: 

or, even better, keep them separated but still add the comments in each (should not be too hard, with git rebase -i)

Sandro Santilli <strk>
Group Member
Mon 21 Sep 2015 07:55:51 AM UTC, comment #7: 

Wow, lots of patches.
The signed vs. unsigned change really deserves some inline comment otherwise it'll be hard for anyone reading the code to understand why an unsigned codec is used for an advertised-as-signed one.

What about merging all patches in a single one and adding proper comments to explain what's being done ?

This is particularly needed as we have NO AUTOMATED TESTS to ensure a regression is not introduced.

Sandro Santilli <strk>
Group Member
Sun 20 Sep 2015 06:59:38 AM UTC, comment #6: 

These patches are tested using PCM event sound with every combination
of `UNCOMPRESSED`/`RAW` (Little Endian) format, 44100 Hz/22050 Hz/11025 Hz
sample rate, 16-bit/8-bit depth, and 2/1 channels. All combination
used 1.0 sec (exact) audio length.

(PCM codec test files, including original sounds and build scripts,
are already attached to bug #45722 as `pcmplay*.zip`)

I have also briefly tested with other event sound codecs:

  • MP3: all combination of 44100 Hz/22050 Hz/11025 Hz sample rate, 1 and 2 channels. (1.0 sec length)
  • IMA-ADPCM: 44100 Hz, 2 channels with 2/3/4/5 bits predictor. (1.0 sec length)


and some stream sound codecs:

  • `RAW` PCM: 22050 Hz 8-bit 2 channels (6.0 sec length, 12 fps)
  • MP3: 44100 Hz 2 channels CBR 128kbps (6.0 sec length, 12 fps)
  • IMA-ADPCM: 44100 Hz 1 channel with 4-bit predictor (6.0 sec length, 12 fps)


No side effects have been found so far.

Note: Speex codec is not tested due to the lack of tools (SWFmill currently
doesn't know how to deal with Ogg/Speex file). Asao/Nellymoser codec is not
tested due to the lack of decoding support in my libAV installation
(`gnash --media=gst` played it nicely though).

Gnash: 0.8.11dev (patched against git 516a6ca 15-Sep-2015)
SDL: libsdl1.2debian 1.2.15-5 (debian)
libAV: libavcodec53 6:0.8.17-1 (debian), libavformat53 6:0.8.17-1 (debian), libavutil51 6:0.8.17-1 (debian)
CSound: libcsound64-5.2 1:5.17.11~dfsg-3 (debian)
System: Debian GNU/Linux 7.0 Wheezy i386

Nutchanon Wetchasit <nachanon>
Sun 20 Sep 2015 06:55:24 AM UTC, comment #5: 

`pcmplay-stereo-8bit.swf` from bug #45722 should now be playing properly
with `gnash --media=ffmpeg` once patch 5 is applied.

Gnash: 0.8.11dev (patched against git 516a6ca 15-Sep-2015)
SDL: libsdl1.2debian 1.2.15-5 (debian)
libAV: libavcodec53 6:0.8.17-1 (debian), libavformat53 6:0.8.17-1 (debian), libavutil51 6:0.8.17-1 (debian)
CSound: libcsound64-5.2 1:5.17.11~dfsg-3 (debian)
System: Debian GNU/Linux 7.0 Wheezy i386

Nutchanon Wetchasit <nachanon>
Sun 20 Sep 2015 06:50:50 AM UTC, comment #4: 

Patch 5 of 5:
See the attached `0008-fix-ffmpeg-resample-factor-count-calculation.patch`

Originally, FFmpeg backend used a wrong resample factor calculation
by multiplying in channel expansion to `resampleFactor`; it should
account only sample rate change. The resampled channel number (fixed to 2)
is already multiplied-in later.

This patch removes the channel part from the statement.

Also, current sample count calculation ignores the possibility of the
decoded audio being 1 byte/sample (which is the case when 8-bit
`UNCOMPRESSED`/`RAW PCM audio is being played).

This patch makes the division depends on the decoded sample format.
(Additional divide-by-2 only if the audio is 16-bit or 16-bit planar)

Note: This patch provides no provision for audio codec that decodes into
32/24-bit PCM, 32-bit float, or 64-bit double sample formats. To my knowledge,
Flash didn't really support any codec that decodes into these formats.
(Well, unless Gnash somehow used `ffmp3float` decoder for MP3 codec.
Original code didn't have any provision for it however)

Gnash: 0.8.11dev (patched against git 516a6ca 15-Sep-2015)
SDL: libsdl1.2debian 1.2.15-5 (debian)
libAV: libavcodec53 6:0.8.17-1 (debian), libavformat53 6:0.8.17-1 (debian), libavutil51 6:0.8.17-1 (debian)
CSound: libcsound64-5.2 1:5.17.11~dfsg-3 (debian)
System: Debian GNU/Linux 7.0 Wheezy i386

(file #34934)

Nutchanon Wetchasit <nachanon>
Sun 20 Sep 2015 06:41:54 AM UTC, comment #3: 

Patch 4 of 5:
See the attached `0007_use-original-sample-rate-for-ffmpeg-8bit-pcm-decoder.patch`

Originally, FFmpeg backend divided sound sample rate by 2 if the source audio
is 8-bit `UNCOMPRESSED`/`RAW` PCM, for an unobvious reason.

This patch removes the divisor; allowing the resampler invoker to use
a correct sample rate for resampling factor calculation.

Note: `pcmplay-8bit.swf` from bug #45722 should now be playing properly
with `gnash --media=ffmpeg` once this patch is applied.

Gnash: 0.8.11dev (patched against git 516a6ca 15-Sep-2015)
SDL: libsdl1.2debian 1.2.15-5 (debian)
libAV: libavcodec53 6:0.8.17-1 (debian), libavformat53 6:0.8.17-1 (debian), libavutil51 6:0.8.17-1 (debian)
CSound: libcsound64-5.2 1:5.17.11~dfsg-3 (debian)
System: Debian GNU/Linux 7.0 Wheezy i386

(file #34933)

Nutchanon Wetchasit <nachanon>
Sun 20 Sep 2015 06:33:20 AM UTC, comment #2: 

Patch 3 of 5:
See the attached `0006_use-correct-8bit-pcm-ffmpeg-codec.patch`

Originally, FFmpeg backend mistakenly used a signed 8-bit PCM codec ID
when decoding Flash's 8-bit PCM `UNCOMPRESSED`/`RAW` sound, which is actually
an unsigned 8-bit PCM data.

This patch corrects it by using an unsigned PCM 8-bit codec ID
(`AV_CODEC_ID_PCM_U8`) instead.

Gnash: 0.8.11dev (patched against git 516a6ca 15-Sep-2015)
SDL: libsdl1.2debian 1.2.15-5 (debian)
libAV: libavcodec53 6:0.8.17-1 (debian), libavformat53 6:0.8.17-1 (debian), libavutil51 6:0.8.17-1 (debian)
CSound: libcsound64-5.2 1:5.17.11~dfsg-3 (debian)
System: Debian GNU/Linux 7.0 Wheezy i386

(file #34932)

Nutchanon Wetchasit <nachanon>
Sun 20 Sep 2015 06:28:57 AM UTC, comment #1: 

Patch 2 of 5:
See the attached `0005_dont-skip-resampling-8bit-audio.patch`

Originally, FFmpeg backend ignored non-16 bit audio when checking whether
it needed to call FFmpeg/libAV audio resampler, if user's
FFmpeg/libAV version didn't have both `swresample.h` and `avresample.h`.

This patch makes sure it always consider the possibility that source sample
format is not 16-bit (which is the case when original sound data is 8-bit PCM),
and invoke the resampler accordingly.

Gnash: 0.8.11dev (patched against git 516a6ca 15-Sep-2015)
SDL: libsdl1.2debian 1.2.15-5 (debian)
libAV: libavcodec53 6:0.8.17-1 (debian), libavformat53 6:0.8.17-1 (debian), libavutil51 6:0.8.17-1 (debian)
CSound: libcsound64-5.2 1:5.17.11~dfsg-3 (debian)
System: Debian GNU/Linux 7.0 Wheezy i386

(file #34931)

Nutchanon Wetchasit <nachanon>
Sun 20 Sep 2015 06:17:21 AM UTC, original submission:  

This series of patches fixes FFmpeg backend-specific issue of bug #45722
(PCM embeded sound screeching) in 8-bit PCM audio case.

Note #1: There are multiple issues in Gnash's FFmpeg codec and resampler
backend which are closely tangled together. As these patches tries to fix
each of them individually, the symptom will change or worsen as the patching go,
but will get better at the end where all the patches are completely applied.

Note #2: I haven't tested on multiple variants of FFmpeg/libAV (especially
ones that came with `swresample.h` or `avresample.h`). So it would need
a more intensive review and testing to make sure that this series of fix
doesn't introduce regression on other FFmpeg/libAV version or other non-PCM codecs.


Patch 1 of 5:
See the attached `0004_use-source-sample-format-in-ffmpeg-audio-resampler.patch`

Originally, FFmpeg backend's used a hard-coded source sample format
(signed PCM 16-bit) when calling FFmpeg/libAV audio resampler, if user's
FFmpeg/libAV version didn't have both `swresample.h` and `avresample.h`.

This patch makes it use the original decoded sample format instead.
(Which could be PCM 8-bit if the original sound is 8-bit PCM
in `UNCOMPRESSED` or `RAW` format)

Note: If `swresample.h` and/or `avresample.h` is available, original
sample format is already used in the resampler invocation.

Gnash: 0.8.11dev (patched against git 516a6ca 15-Sep-2015)
SDL: libsdl1.2debian 1.2.15-5 (debian)
libAV: libavcodec53 6:0.8.17-1 (debian), libavformat53 6:0.8.17-1 (debian), libavutil51 6:0.8.17-1 (debian)
CSound: libcsound64-5.2 1:5.17.11~dfsg-3 (debian)
System: Debian GNU/Linux 7.0 Wheezy i386

Nutchanon Wetchasit <nachanon>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attached Files
file #34960:  0008-fix-ffmpeg-resample-factor-count-calculation-v2.patch added by nachanon (2KiB - text/x-patch - Patch correct resampling ratio and sample count FFmpeg/libAV-based audio decoder; annotated version)
file #34959:  0007_use-original-sample-rate-for-ffmpeg-8bit-pcm-decoder-v2.patch added by nachanon (865B - text/x-patch - Patch to make FFmpeg/libAV-based audio decoder use the original sample rate for 8-bit PCM sound; rebased version)
file #34958:  0006_use-correct-8bit-pcm-ffmpeg-codec-v2.patch added by nachanon (2KiB - text/x-patch - Patch to make FFmpeg/libAV-based audio decoder use a correct codec ID for 8-bit PCM sound; annotated version)
file #34934:  0008-fix-ffmpeg-resample-factor-count-calculation.patch added by nachanon (1KiB - text/x-patch - Patch correct resampling ratio and sample count FFmpeg/libAV-based audio decoder)
file #34933:  0007_use-original-sample-rate-for-ffmpeg-8bit-pcm-decoder.patch added by nachanon (865B - text/x-patch - Patch to make FFmpeg/libAV-based audio decoder use the original sample rate for 8-bit PCM sound)
file #34932:  0006_use-correct-8bit-pcm-ffmpeg-codec.patch added by nachanon (2KiB - text/x-patch - Patch to make FFmpeg/libAV-based audio decoder use a correct codec ID for 8-bit PCM sound)
file #34931:  0005_dont-skip-resampling-8bit-audio.patch added by nachanon (797B - text/x-patch - Patch to make FFmpeg/libAV-based resampler always check if source audio is not 16-bit)
file #34930:  0004_use-source-sample-format-in-ffmpeg-audio-resampler.patch added by nachanon (793B - text/x-patch - Patch to make FFmpeg/libAV-based resampler consider decoded sample format in all cases)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by strk (Posted a comment)
  • -email is unavailable- added by nachanon (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

     

    Follow 11 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2015-09-23 strk StatusNone Done
        Open/ClosedOpen Closed
    2015-09-23 nachanon Attached File- Added 0008-fix-ffmpeg-resample-factor-count-calculation-v2.patch, #34960
    2015-09-23 nachanon Attached File- Added 0007_use-original-sample-rate-for-ffmpeg-8bit-pcm-decoder-v2.patch, #34959
    2015-09-23 nachanon Attached File- Added 0006_use-correct-8bit-pcm-ffmpeg-codec-v2.patch, #34958
    2015-09-23 strk Assigned toNone strk
    2015-09-20 nachanon Attached File- Added 0008-fix-ffmpeg-resample-factor-count-calculation.patch, #34934
    2015-09-20 nachanon Attached File- Added 0007_use-original-sample-rate-for-ffmpeg-8bit-pcm-decoder.patch, #34933
    2015-09-20 nachanon Attached File- Added 0006_use-correct-8bit-pcm-ffmpeg-codec.patch, #34932
    2015-09-20 nachanon Attached File- Added 0005_dont-skip-resampling-8bit-audio.patch, #34931
    2015-09-20 nachanon Attached File- Added 0004_use-source-sample-format-in-ffmpeg-audio-resampler.patch, #34930

    Back to the top

    Powered by Savane 3.12.
    Corresponding source code