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

Submitted by:  Nutchanon Wetchasit <nachanon>
Submitted on:  Sun 20 Sep 2015 06:17:21 AM UTC  
 
Category: soundPriority: 5 - Normal
Status: DonePrivacy: Public
Assigned to: Sandro Santilli <strk>Open/Closed: Closed

(Jump to the original submission Jump to the original submission)

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

Patches here all pushed.

Sandro Santilli <strk>
Project MemberIn charge of this item.
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>
Project MemberIn charge of this item.
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:

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:

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>
Project MemberIn charge of this item.
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>
Project MemberIn charge of this item.
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>

 

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
  • -unavailable- added by strk (Posted a comment)
  • -unavailable- added by nachanon (Submitted the item)
  •  

    Do you think this task is very important?
    If so, you can click here to add your encouragement to it.
    This task has 0 encouragements so far.

    Only logged-in users can vote.

     

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    Follow 11 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Wed 23 Sep 2015 01:47:44 PM UTCstrkStatusNone=>Done
      Open/ClosedOpen=>Closed
    Wed 23 Sep 2015 10:24:01 AM UTCnachanonAttached File-=>Added 0008-fix-ffmpeg-resample-factor-count-calculation-v2.patch, #34960
    Wed 23 Sep 2015 10:23:23 AM UTCnachanonAttached File-=>Added 0007_use-original-sample-rate-for-ffmpeg-8bit-pcm-decoder-v2.patch, #34959
    Wed 23 Sep 2015 10:22:13 AM UTCnachanonAttached File-=>Added 0006_use-correct-8bit-pcm-ffmpeg-codec-v2.patch, #34958
    Wed 23 Sep 2015 06:21:28 AM UTCstrkAssigned toNone=>strk
    Sun 20 Sep 2015 06:50:50 AM UTCnachanonAttached File-=>Added 0008-fix-ffmpeg-resample-factor-count-calculation.patch, #34934
    Sun 20 Sep 2015 06:41:54 AM UTCnachanonAttached File-=>Added 0007_use-original-sample-rate-for-ffmpeg-8bit-pcm-decoder.patch, #34933
    Sun 20 Sep 2015 06:33:20 AM UTCnachanonAttached File-=>Added 0006_use-correct-8bit-pcm-ffmpeg-codec.patch, #34932
    Sun 20 Sep 2015 06:28:57 AM UTCnachanonAttached File-=>Added 0005_dont-skip-resampling-8bit-audio.patch, #34931
    Sun 20 Sep 2015 06:17:21 AM UTCnachanonAttached File-=>Added 0004_use-source-sample-format-in-ffmpeg-audio-resampler.patch, #34930

    Back to the top


    Powered by Savane 3.1-cleanup1