patchGnash - The GNU Flash player - Patches: patch #8734, Patches for improving...

 
 

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

patch #8734: Patches for improving `--audio-dump` support in Gnash

Submitter:  Nutchanon Wetchasit <nachanon>
Submitted:  Mon 07 Sep 2015 09:31:43 AM UTC
   
 
Category:  sound Priority:  5 - Normal
Status:  In Progress Privacy:  Public
Assigned to:  strk Open/Closed:  Open

Jump to the original submission

Thu 26 Jan 2017 11:03:54 AM UTC, comment #10: 

Patch 4/4 applied with git.savannah.gnu.org/cgit/gnash.git/commit/?id=f9db2f227e72b177854f497b72c0f9ab45b6869a

The output message change included in Patch 3/4 (pushed with git.savannah.gnu.org/cgit/gnash.git/commit/?id=2f7d9b270b6ae90ef1af5ceb4ad6a49d5a053c04) should probably still be reverted to avoid breaking scripted usages of dump-gnash, which would use the output as a shell script setting variables (AUDIOFILE=xxxx; # comment).


Sandro Santilli <strk>
Group Member
Thu 01 Oct 2015 04:52:06 PM UTC, comment #9: 

Patch 4 of 4: see the attached `0004_use-endian-neutral-wave-header-write.patch`

Original code assumes the platform is using little-endian byte order
when it writes WAVE header, which would produce an unreadable file if
the system is big-endian.

This patch switches it to an endian-neutral bit shifting. (Might need a review,
since I'm not sure if there is a less clunky way to do this)

Note: The patch does not provide a provision for audio data's byte ordering itself.

Gnash: 0.8.11dev (patched against git f0f66ce 23-Sep-2015)
System: Debian GNU/Linux 7.0 Wheezy i386

(file #35023)

 Nutchanon Wetchasit <nachanon>
Wed 09 Sep 2015 11:48:22 AM UTC, comment #8: 

I think the output message was meant to be used by shell scripts, notice the comment in first line and the variable set in second line

 Sandro Santilli <strk>
Group Member
Wed 09 Sep 2015 11:25:44 AM UTC, comment #7: 

Patch 3: see the attached `0003_fix-audio-dump-message.patch`

This patch corrects a wrong audio dump notice message, and also change
the way it was emitted, from `cout` to a proper `log_debug()`.

(file #34839)

 Nutchanon Wetchasit <nachanon>
Tue 08 Sep 2015 08:12:52 PM UTC, comment #6: 

I've changed the comment as described below in c969f9d9d9a.

 Bastiaan Jacques <bjacques>
Group Member
Mon 07 Sep 2015 07:30:37 PM UTC, comment #5: 

both patches pushed to master

 Sandro Santilli <strk>
Group Member
Mon 07 Sep 2015 07:17:21 PM UTC, comment #4: 

actually, don't bother, I'm building and will push as soon as quickly tested.

 Sandro Santilli <strk>
Group Member
Mon 07 Sep 2015 07:14:23 PM UTC, comment #3: 

Can you go ahead and commit them Bastiaan ? Or I'll try to find the time myself ...

 Sandro Santilli <strk>
Group Member
Mon 07 Sep 2015 06:25:11 PM UTC, comment #2: 


@@ -75,7 +76,19 @@ WAVWriter::WAVWriter(const std::string& wavefile)
 /* public */
 WAVWriter::~WAVWriter()
 {
-    if (file_stream) file_stream.close();
+    if (file_stream) {
+        // attempt to flush metadata


I think this comment should correspond to what is actually happening here, i.e. something like:

// Seeking back to the beginning, in order to rewrite the
// header with information accumulated during writing.


+        file_stream.seekp(0);
+        if (file_stream.fail()) {
+            log_error("WAVWriter: Failed to flush audio dump metadata, resulting file would be incomplete");
+        }
+        else {
+            write_wave_header(file_stream);
+        }
+
+        // close the stream
+        file_stream.close();
+    }
 }


Aside from that, both patches look good to me.

 Bastiaan Jacques <bjacques>
Group Member
Mon 07 Sep 2015 11:11:57 AM UTC, comment #1: 

Patch 2 of 4: see the attahced `0002_flush-wave-metadata-on-audio-dump-end.patch`.

This patch addresses the issue of an un-updated "data" and "RIFF" chunk length,
by making `WAVWriter`'s destructor seek back and re-write WAVE header when
the dumping ended (user closed the window, clicked File > Quit, or
Gnash reached the specified frame), before closing the output stream.

Might need a review since I don't know much about Gnash's convention
regarding log message.

This completes the fix of main issue in bug #45887.

Caveat: This patch provides no provision for signal handling; terminating
Gnash with Ctrl+C, Ctrl+\, or using `kill` will still result in an incorrect
dump file (though still recoverable using FFmpeg/AVconv).

Gnash: 0.8.11dev (patched against git dc51f53 28-Aug-2015)
System: Debian GNU/Linux 7.0 Wheezy i386

(file #34824)

 Nutchanon Wetchasit <nachanon>
Mon 07 Sep 2015 09:31:43 AM UTC, original submission:  

Gnash's support for `--audio-dump`/`-A` option have some problems that
rendered output file unreadable by several program. This narrowed down to
how Gnash writes RIFF WAVE metadata in `libsound/WAVWriter.cpp`.

Attached patch (patch 1 of 4: `0001_use-correct-riff-wave-chunk-size.patch`)
addresses the issue of a wrong RIFF chunk size in output WAVE file. (Which was
originally off-by-8 because "RIFF" string and its chunk size field was
erroneously included in the count).

This partially fixes bug #45887.

Gnash: 0.8.11dev (patched against git dc51f53 28-Aug-2015)
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 #35023:  0004_use-endian-neutral-wave-header-write.patch added by nachanon (2KiB - text/x-patch - Patch make Gnash always use a correct byte order on writing WAVE header)
file #34839:  0003_fix-audio-dump-message.patch added by nachanon (686B - text/x-patch - Patch to correct audio dumping message)
file #34824:  0002_flush-wave-metadata-on-audio-dump-end.patch added by nachanon (3KiB - text/x-patch - Patch to make Gnash update WAVE header before exiting)
file #34823:  0001_use-correct-riff-wave-chunk-size.patch added by nachanon (630B - text/x-patch - Patch to correct RIFF chunk size in WAVE dump file)

 

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 bjacques (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 10 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2015-10-01 nachanon Attached File- Added 0004_use-endian-neutral-wave-header-write.patch, #35023
    2015-09-09 nachanon Attached File- Added 0003_fix-audio-dump-message.patch, #34839
    2015-09-08 bjacques StatusDone In Progress
        Open/ClosedClosed Open
    2015-09-07 strk StatusIn Progress Done
        Open/ClosedOpen Closed
    2015-09-07 strk StatusNone In Progress
        Assigned toNone strk
    2015-09-07 nachanon Attached File- Added 0002_flush-wave-metadata-on-audio-dump-end.patch, #34824
    2015-09-07 nachanon Attached File- Added 0001_use-correct-riff-wave-chunk-size.patch, #34823

    Back to the top

    Copyright © 2024 Free Software Foundation, Inc.
    Verbatim copying and distribution of this entire article is permitted in any medium, provided this notice is preserved.
    The Levitating, Meditating, Flute-playing Gnu logo is a GNU GPL'ed image provided by the Nevrax Design Team.
    Page source code

    Powered by Savane 3.13-df36.
    Corresponding source code