bugGnash - The GNU Flash player - Bugs: bug #46607, Embedded audio quality degradation

 
 

bug #46607: Embedded audio quality degradation

Submitter:  Sandro Santilli <strk>
Submitted:  Fri 04 Dec 2015 10:44:34 PM UTC
   
 
Category:  sound Severity:  5 - Blocker
Release:  master Status:  Fixed
Privacy:  Public Assigned to:  strk
Open/Closed:  Closed
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Wed 16 Dec 2015 10:19:32 AM UTC, comment #19: 

I think the audio degradation issue is gone, so closing.

Sandro Santilli <strk>
Group Member
Wed 16 Dec 2015 08:53:27 AM UTC, comment #18: 

Filed another ticket for the missing sound: #46693

Sandro Santilli <strk>
Group Member
Tue 15 Dec 2015 05:04:17 PM UTC, comment #17: 

I've found another regression in sound handling, namely sounds that are missing from output. I'm doing further research.
Commit b7a877c works fine, so it must be another one after that.

Sandro Santilli <strk>
Group Member
Tue 08 Dec 2015 11:57:17 AM UTC, comment #16: 

On the other hand, in the tennis.swf test, Bastiaan code correctly emits the "bouncing tennis ball" sounds when hovering on the Play/Practice buttons, while current master does not.

Sandro Santilli <strk>
Group Member
Tue 08 Dec 2015 11:44:33 AM UTC, comment #15: 

I found another test showing a difference of behavior with Bastiaan code and w/out:
http://strk.keybit.net/tmp/tennis.swf

The music at start is supposed to continue/loop while it stops shortly after starting with Bastiaan code (as patched with my patch).

Maybe it's not really worth reverting the revert until we can perform more tests...

Sandro Santilli <strk>
Group Member
Tue 08 Dec 2015 11:25:26 AM UTC, comment #14: 

Sorry for the naming, the method patched is "consumeInPoint", not "consumeInput".

Sandro Santilli <strk>
Group Member
Tue 08 Dec 2015 11:24:24 AM UTC, comment #13: 

I've looked closely at bastiann code and found the problem being in the LiveSound::consumeInput function. I have a patch that fixes that implementation so we might go back to the new implementation, but would like a review from Bastiaan.

The patch applies to the code before the revert.
I'm attaching it here (consumeInput.patch).

Basically the "delaySeek" value is larger than the size of he first buffer, so setting _consumed to the requested size is incorrect, and leaves _pos in a wrong state.

Bastiaan, if you think the new code is worth it, I'd revert the revert and apply this patch. I dunno if you run performance tests to check how the new code compared to the old...

(file #35661)

Sandro Santilli <strk>
Group Member
Mon 07 Dec 2015 05:45:36 PM UTC, comment #12: 

I found a shareable example of a streaming mp3 sound showing the degradation. Not very focused but it's a start.

Alfy Havoc: http://strk.keybit.net/tmp/alfy-havoc.swf

Start the video, click on the coo-coo clock when asked to
and hear to the phrase pronounced by Alfy:

  "It's 3 o'clock, let's go get my puppy"

It sounds badly with eda27746fcd190fc4092e0cb1166726bebf000df (before reverting bastiaan change) and it is fixed by the revert in commit 41ae739439c796f7f310f813f18cc12287b2e450

Sandro Santilli <strk>
Group Member
Mon 07 Dec 2015 04:42:49 PM UTC, comment #11: 

@strk's follow-ups on this issue somehow ended up in patch #8721,
I took a liberty to quote them here for continuity:

Comment 12:

Bastiaan the consumeInPoint method in your change looks suspicious
as you're setting _pos only at the final iteration of the loop,
setting it to the advancement within the last buffer, rather than
advancing all the buffers.


Comment 13:

Also, _pos is never updated because there's a single buffer whose
size() is < _in_point


Nutchanon Wetchasit <nachanon>
Mon 07 Dec 2015 03:18:10 PM UTC, comment #10: 

latencySeek in the SoundStreamHead seem to make a difference between playing nicely (with no latencySeek) and badly (w/out).
This is when NOT reverting bastiaan commit.

Sandro Santilli <strk>
Group Member
Mon 07 Dec 2015 02:45:02 PM UTC, comment #9: 

reverted with 41ae739439c796f7f310f813f18cc12287b2e450 -- I'm still working on a reduced testcase (not automated).

Sandro Santilli <strk>
Group Member
Sat 05 Dec 2015 04:24:27 PM UTC, comment #8: 

No, go ahead.

Bastiaan Jacques <bjacques>
Group Member
Sat 05 Dec 2015 04:14:16 PM UTC, comment #7: 

I tried a revert and it does fix the quality issue.
Will now look for a way to provide a simple testcase.
Bastiaan, do you have any objection about reverting that commit ?

Sandro Santilli <strk>
Group Member
Fri 04 Dec 2015 11:04:54 PM UTC, comment #6: 

Both gstreamer and ffmpeg media handlers are affected (as expected)

Sandro Santilli <strk>
Group Member
Fri 04 Dec 2015 11:03:45 PM UTC, comment #5: 

Ok, I do can confirm. Sorry for the double-fail, too many build directories around.

Sandro Santilli <strk>
Group Member
Fri 04 Dec 2015 11:02:30 PM UTC, comment #4: 

BUT, I cannot confirm this commit was really the culprit :/

Sandro Santilli <strk>
Group Member
Fri 04 Dec 2015 11:01:40 PM UTC, comment #3: 

I think a "realloc" should reduce the pressure by a huge degree, if the problam was the new allocation and the copy. Internally, the virtual memory subsystem would just do what you attempted to do via C++ code.

Sandro Santilli <strk>
Group Member
Fri 04 Dec 2015 10:55:38 PM UTC, comment #2: 

The embedded sound is an mp3, single HEAD and 223 BLOCKS.

Block type: 18 (SWF_SOUNDSTREAMHEAD)
Block length: 6

  PlaybackSoundRate 44 kHz
  PlaybackSoundSize 16 bit
  PlaybackSoundType mono
  StreamSoundCompression MP3
  StreamSoundRate 44 kHz
  StreamSoundSize 16 bit
  StreamSoundType mono
  StreamSoundSampleCount 1837
  LatencySeek 1633

Sandro Santilli <strk>
Group Member
Fri 04 Dec 2015 10:44:59 PM UTC, comment #1: 

This is actually a blocker, being a regression

Sandro Santilli <strk>
Group Member
Fri 04 Dec 2015 10:44:34 PM UTC, original submission:  

I've found a movie which plays stuttering and bisecting got to this point:

a9f7f743c66f2f36d81c348da238d94877473372 is the first bad commit
commit a9f7f743c66f2f36d81c348da238d94877473372
Author: Bastiaan Jacques <bastiaan@bjacques.org>
Date:   Sun Jun 15 20:09:12 2014 +0200

    Reduce malloc() churn for SWF-embedded audio.
   
    Decoded data from audio sources stored inside SWFs are decoded and
    stored in a growing buffer. This buffer is appended to as small
    chunks of data become available. In order to store the additional
    data, a new buffer big enough to contain all the data is allocated.
    A minute of audio corresponds to about 10MB of decoded data. The
    ensuing (re)allocation of such a large buffer adds significantly to
    the resource pressure on the system.
   
    This change alters the situation so that instead of allocating a new
    buffer, each buffer is added to a list as-is, at the cost of having
    to keep track of reads in more detail.

:040000 040000 45eed7e653f151eeaf7baf2187d08887b6c169c5 c61e4016939188ae645ff56a9d7a9c972fb241d8 M      libbase
:040000 040000 c5a5e6700aac7e4a084a4fe3fe463469c4cde316 5a86380f1b008ecc77da09db22b10e3bcd61c71e M      libsound

Sandro Santilli <strk>
Group Member

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #35661:  consumeInput.patch added by strk (1001B - text/x-diff)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by nachanon (Posted a comment)
  • -email is unavailable- added by bjacques (Posted a comment)
  • -email is unavailable- added by strk (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.

    Only logged-in users can vote.

     

    Follow 6 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2015-12-16 strk StatusReady For Test Fixed
        Open/ClosedOpen Closed
    2015-12-08 strk Attached File- Added consumeInput.patch, #35661
    2015-12-07 strk StatusNone Ready For Test
        Assigned tobjacques strk
    2015-12-04 strk Severity3 - Normal 5 - Blocker

    Back to the top

    Powered by Savane 3.13-3230.
    Corresponding source code