[Gnash-commit] [patch #8734] Patches for improving `--audio-dump` support in Gnash

[Nutchanon Wetchasit]

URL:
<http://savannah.gnu.org/patch/?8734>

Summary: Patches for improving `--audio-dump` support in
Gnash
Project: Gnash - The GNU Flash player
Submitted by: nachanon
Submitted on: Mon 07 Sep 2015 04:31:43 PM ICT
Category: sound
Priority: 5 - Normal
Status: None
Privacy: Public
Assigned to: None
Originator Email:
Open/Closed: Open
Discussion Lock: Any

_______________________________________________________

Details:

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`
<http://git.savannah.gnu.org/cgit/gnash.git/tree/libsound/WAVWriter.cpp?id=dc51f5389e4726486b631864151dcfe28e556cf6>.

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
<https://savannah.gnu.org/bugs/?45887#comment3>).

This partially fixes bug #45887.

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



_______________________________________________________

File Attachments:


-------------------------------------------------------
Date: Mon 07 Sep 2015 04:31:44 PM ICT Name:
0001_use-correct-riff-wave-chunk-size.patch Size: 630B By: nachanon
Patch to correct RIFF chunk size in WAVE dump file
<http://savannah.gnu.org/patch/download.php?file_id=34823>

_______________________________________________________

Reply to this item at:

<http://savannah.gnu.org/patch/?8734>

_______________________________________________
Message sent via/by Savannah
http://savannah.gnu.org/

[Nutchanon Wetchasit]

Follow-up Comment #1, patch #8734 (project gnash):

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
<https://savannah.gnu.org/bugs/?45887#comment3>,
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)
_______________________________________________________

Additional Item Attachment:

File name: 0002_flush-wave-metadata-on-audio-dump-end.patch Size:2 KB


_______________________________________________________

Reply to this item at:

<http://savannah.gnu.org/patch/?8734>

_______________________________________________
Message sent via/by Savannah
http://savannah.gnu.org/

[Bastiaan Jacques]

Follow-up Comment #2, patch #8734 (project gnash):


@@ -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.

_______________________________________________________

Reply to this item at:

<http://savannah.gnu.org/patch/?8734>

_______________________________________________
Message sent via/by Savannah
http://savannah.gnu.org/

[Sandro Santilli]

Follow-up Comment #3, patch #8734 (project gnash):

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

_______________________________________________________

Reply to this item at:

<http://savannah.gnu.org/patch/?8734>

_______________________________________________
Message sent via/by Savannah
http://savannah.gnu.org/

[Sandro Santilli]

Update of patch #8734 (project gnash):

Status: None => In Progress
Assigned to: None => strk

_______________________________________________________

Follow-up Comment #4:

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

_______________________________________________________

Reply to this item at:

<http://savannah.gnu.org/patch/?8734>

_______________________________________________
Message sent via/by Savannah
http://savannah.gnu.org/

[Sandro Santilli]

Update of patch #8734 (project gnash):

Status: In Progress => Done
Open/Closed: Open => Closed

_______________________________________________________

Follow-up Comment #5:

both patches pushed to master

_______________________________________________________

Reply to this item at:

<http://savannah.gnu.org/patch/?8734>

_______________________________________________
Message sent via/by Savannah
http://savannah.gnu.org/

[Bastiaan Jacques]

Update of patch #8734 (project gnash):

Status: Done => In Progress
Open/Closed: Closed => Open

_______________________________________________________

Follow-up Comment #6:

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

_______________________________________________________

Reply to this item at:

<http://savannah.gnu.org/patch/?8734>

_______________________________________________
Message sent via/by Savannah
http://savannah.gnu.org/

[Nutchanon Wetchasit]

Follow-up Comment #7, patch #8734 (project gnash):

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)
_______________________________________________________

Additional Item Attachment:

File name: 0003_fix-audio-dump-message.patch Size:0 KB


_______________________________________________________

Reply to this item at:

<http://savannah.gnu.org/patch/?8734>

_______________________________________________
Message sent via/by Savannah
http://savannah.gnu.org/

[Sandro Santilli]

Follow-up Comment #8, patch #8734 (project gnash):

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

_______________________________________________________

Reply to this item at:

<http://savannah.gnu.org/patch/?8734>

_______________________________________________
Message sent via/by Savannah
http://savannah.gnu.org/

[Nutchanon Wetchasit]

Follow-up Comment #9, patch #8734 (project gnash):

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
<https://savannah.gnu.org/bugs/?45887#comment3>
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)
_______________________________________________________

Additional Item Attachment:

File name: 0004_use-endian-neutral-wave-header-write.patch Size:1 KB


_______________________________________________________

Reply to this item at:

<http://savannah.gnu.org/patch/?8734>

_______________________________________________
Message sent via/by Savannah
http://savannah.gnu.org/

[Sandro Santilli]

Follow-up Comment #10, patch #8734 (project gnash):

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).



_______________________________________________________

Reply to this item at:

<http://savannah.gnu.org/patch/?8734>

_______________________________________________
Message sent via/by Savannah
http://savannah.gnu.org/