patchGnash - The GNU Flash player - Patches: patch #7575, No memcpy when endian-swapping...

 
 

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

patch #7575: No memcpy when endian-swapping input

Submitter:  Jonas 'Sortie' Termansen <sortie>
Submitted:  Sun 10 Jul 2011 10:22:29 PM UTC
   
 
Category:  sound Priority:  3 - Low
Status:  Done Privacy:  Public
Assigned to:  strk Open/Closed:  Closed

Jump to the original submission

Tue 12 Jul 2011 07:48:05 PM UTC, comment #7: 

No, little-endian only, but still saw your patch (in addition to 55a441446f) being required to get any sound out.

Did you test it ?

Sandro Santilli <strk>
Group Member
Tue 12 Jul 2011 06:59:59 PM UTC, comment #6: 

Did you test on a Big-Endian CPU? I only touched the BE path of execution.

Jonas 'Sortie' Termansen <sortie>
Group Member
Tue 12 Jul 2011 08:31:33 AM UTC, comment #5: 

Alright, manual test shows that sortie's patch actually fixes playback of uncompressed sound here.

Pushed to master as bfc794c3074ad787031015d456209f5c3b3c1818.

The testcases used for manual test were:
 http://www.haxer.be/guests/gnash/Uncompressed-LE-SoundFormat3-Stream.swf
 http://www.haxer.be/guests/gnash/Uncompressed-LE-SoundFormat3-Event.swf

Given the complexity of producing automated testcases (lack of framework ready to host them) I tought it was good to have this in nonetheless. Implementation of a proper framework will have to be dealt with as a separate task.

Sandro Santilli <strk>
Group Member
Tue 12 Jul 2011 08:15:59 AM UTC, comment #4: 

Ok, as of 55a441446fb57851dcc61a950083f9726df613f0, the GST MediaHandler will successfully create an audio decoder for UNCOMPRESSED sound, using AudioDecoderSimple.

So, in order to test this patch "manually", you have to make sure you use gst media (-M gst) and to be sure your GST media isn't triggering some decoder within its plugins (check with -vv).

"Automatic" testing will be much harder indeed, in that we still don't even test both gst and ffmpeg automatically (but only test whatever happens to be the default media handler instead, which is ffmpeg when both are built).

Sandro Santilli <strk>
Group Member
Mon 11 Jul 2011 04:20:34 PM UTC, comment #3: 

Note that ffmpeg uses its own decoder for UNCOMPRESSED sound while GST seems to be just giving up after attempting to find a proper GST decoder:


ERROR: AudioDecoder initialization failed: MediaHandlerGst::createAudioDecoder: AudioDecoderGst: cannot handle codec 3 (Uncompressed) -- MediaHandler::createFlashAudioDecoder: no available flash decoders for codec 3 (Uncompressed)


So it sounds like currently AudioDecoderSimple is not even possible to be used ! I think the GST one might be a bug (could fallback to AudioDecoderSimple)

Sandro Santilli <strk>
Group Member
Mon 11 Jul 2011 02:33:37 PM UTC, comment #2: 

I've verified there's currently no test for raw sound in our testsuite.

Sandro Santilli <strk>
Group Member
Mon 11 Jul 2011 01:58:38 PM UTC, comment #1: 

It is very important when doing work on performance optimization to make sure that the change doesn't break anything. For sound this is particularly hard as we don't have many tests.

So, how would you test this ? Are there tests already in our testsuite triggering execution of the code you're touching ?
If not, you should add some.

Sandro Santilli <strk>
Group Member
Sun 10 Jul 2011 10:22:29 PM UTC, original submission:  

Just a small inefficiency I found when looking over AudioDecoderSimple. It copied the input data to the output buffer, and then endian-swapped the entire buffer. This results in 2 read, 1 swap, and 2 write operations for each 16-bit word.

Instead, by manually copying the data and swapping during the copy, this can be done with 1 read, 1 swap, and 1 write operation for each 16-bit word.

I attached the patch below in git format-patch format. (Contact me on IRC if you want me to start my git server -- I don't run it 24/7).

Jonas 'Sortie' Termansen <sortie>
Group Member

 

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

 

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 sortie (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 5 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2011-07-12 strk StatusIn Progress Done
        Open/ClosedOpen Closed
    2011-07-11 strk StatusNone In Progress
        Assigned toNone strk
    2011-07-10 sortie Attached File- Added 0001-Don-t-memcpy-data-if-you-need-to-endian-swap-it-anyw.patch, #23637

    Back to the top

    Powered by Savane 3.13-4b48.
    Corresponding source code