patchGnash - The GNU Flash player - Patches: patch #8993, Patches to fix problems when...

 
 

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

patch #8993: Patches to fix problems when `--disable-media` configure option was used

Submitter:  Nutchanon Wetchasit <nachanon>
Submitted:  Sat 07 May 2016 02:59:36 AM UTC
   
 
Category:  sound Priority:  5 - Normal
Status:  Done Privacy:  Public
Assigned to:  strk Open/Closed:  Closed

Jump to the original submission

Wed 11 May 2016 06:24:16 PM UTC, comment #14: 

Thanks for confirming (and for the patches!)

Sandro Santilli <strk>
Group Member
Wed 11 May 2016 04:17:40 PM UTC, comment #13: 

On current Gnash git version, build errors and crashes are now fixed. Thanks.

Gnash: 0.8.11dev (git c6da486 10-May-2016)
System: Debian GNU/Linux 7.0 Wheezy i386

Nutchanon Wetchasit <nachanon>
Wed 11 May 2016 01:10:21 PM UTC, comment #12: 

I actually like to keep debugging output fully instructive.
As of commit c6da4863e343fd228e5546ec053b23ff1c3794de
your two patches are in and I added to those some warnings removal.

See how you like it.

Sandro Santilli <strk>
Group Member
Tue 10 May 2016 05:59:28 PM UTC, comment #11: 

The new version of patch 1 adds return value fixup, changes log message display, and drops libgnashmedia MediaParser operator move.

Updated patch series for current 0.8.11dev master version:

  • 1/2: 0001_fix-disable-media-compile-link-failure-v2.patch (file #37120)
  • 2/2: 0002_refuse-invoking-nonexistent-media-handler.patch (unchanged, file #37094)


This patch series is tested with following scenarios:

  • SWF with embeded event sound (PCM 44100 Hz 16-bit Mono, 1.0 sec)
  • SWF with embeded frame-synchronized stream sound (MP3 22050 Hz Stereo 64kbps, 10.0 sec)
  • SWF playing external event MP3 sound via `Sound.loadSound()` (MP3 44100 Hz Mono 32kbps CBR, 4.8 sec)
  • SWF playing external streaming MP3 sound loaded via `Sound.loadSound()` (MP3 44100 Hz Mono 32kbps CBR, 4.8 sec)
  • SWF with embeded frame-synchronized video (640x480 24fps FLV1 video, 22050 Hz Mono 32kbps MP3 audio, 10.0 sec)
  • SWF playing external streaming video via `NetStream.play()` (640x480 24fps 128kbps FLV1 video, 44100 Hz Mono 32kbps CBR MP3 audio, FLV container, 10.0 sec)


Gnash: 0.8.11dev (patched against git 484ca4c 29-Mar-2016)
System: Debian GNU/Linux 7.0 Wheezy i386

Nutchanon Wetchasit <nachanon>
Tue 10 May 2016 05:53:23 PM UTC, comment #10: 

@strk:
Hmm, not really; at the time of previous post, I already have a working patch. The fix looks like this:

--- a/libcore/swf/SoundStreamHeadTag.cpp
+++ b/libcore/swf/SoundStreamHeadTag.cpp
@@ -146,8 +146,14 @@ SoundStreamHeadTag::loader(SWFStream& in, TagType tag, movie_definition& m,

     IF_VERBOSE_PARSE(
         log_parse(_("sound stream head: format=%s, rate=%d, 16=%d, "
-            "stereo=%d, ct=%d, latency=%d"), format, streamSoundRate,
-            +streamSound16bit, +streamSoundStereo, +sampleCount, +latency);
+            "stereo=%d, ct=%d, latency=%d"),
+#ifdef USE_MEDIA
+            format,
+#else
+            std::to_string(static_cast<int>(format)),
+#endif
+            streamSoundRate, +streamSound16bit, +streamSoundStereo,
+            +sampleCount, +latency);
     );

     // Store all the data in a SoundInfo object
--- a/libcore/swf/tag_loaders.cpp
+++ b/libcore/swf/tag_loaders.cpp
@@ -188,8 +188,13 @@ define_sound_loader(SWFStream& in, TagType tag, movie_definition& m,

     IF_VERBOSE_PARSE(
         log_parse(_("define sound: ch=%d, format=%s, rate=%d, 16=%d, "
-            "stereo=%d, ct=%d, delay=%d"), id, format, sample_rate,
-            sample_16bit, stereo, sample_count, delaySeek);
+            "stereo=%d, ct=%d, delay=%d"), id,
+#ifdef USE_MEDIA
+            format,
+#else
+            std::to_string(static_cast<int>(format)),
+#endif
+            sample_rate, sample_16bit, stereo, sample_count, delaySeek);
     );

     // If we have a sound_handler, ask it to init this sound.


I'm personally leaning toward fixing it this way as it is less invasive (and considerably more stright to the point) than the original MediaParser fix.

Gnash: 0.8.11dev (patched against git 484ca4c 29-Mar-2016)
System: Debian GNU/Linux 7.0 Wheezy i386

Nutchanon Wetchasit <nachanon>
Tue 10 May 2016 05:01:52 PM UTC, comment #9: 

Nah, too much work, let's just let it go. After all we're not really expecting anything except gnash to use those libraries.
Let's break the ABI, no problem.

Sandro Santilli <strk>
Group Member
Tue 10 May 2016 03:09:21 PM UTC, comment #8: 

An alternative solution that I could come up with right now is changing the problematic log messages to show just codec number instead of codec name, by casting the `media::videoCodecType format` enum to a generic `int` when `USE_MEDIA` macro is absent.

This way, link errors could be fixed while the useful SWF parsing log messages remain in place and libgnashmedia remains unchanged.

SWF parsing message when run Gnash (verbose parsing) with `pcmplay.swf` (file #34599 bug #45722) on `--disable-media` build would look like this:

define sound: ch=1, format=3, rate=44100, 16=1, stereo=0, ct=44100, delay=0


While on a normal build, the message will look like this:

define sound: ch=1, format=Uncompressed, rate=44100, 16=1, stereo=0, ct=44100, delay=0


SWF parsing message when run Gnash (verbose parsing) with `sync-sound.swf` (file #23431 bug #33294) on `--disable-media` build would look like this:

sound stream head: format=2, rate=22050, 16=1, stereo=1, ct=459, latency=1633


While on a normal build, the message will look like this:

sound stream head: format=MP3, rate=22050, 16=1, stereo=1, ct=459, latency=1633


Gnash: 0.8.11dev (patched against git 484ca4c 29-Mar-2016)
System: Debian GNU/Linux 7.0 Wheezy i386 

Nutchanon Wetchasit <nachanon>
Tue 10 May 2016 03:04:08 PM UTC, comment #7: 

From the code causing link error, they are both SWF parser's logging code; when `log_parse()` tries to convert `media::audioCodecType` to string, it invokes operator `ostream << media::audioCodecType`, which is only implemented in `libgnashmedia.so`.

If I just strip them off using following patch and re-build (with patch 1 but exclude MediaParser inlining), the build will complete without error.

diff --git a/libcore/swf/SoundStreamHeadTag.cpp b/libcore/swf/SoundStreamHeadTag.cpp
index 3b20909..65516a1 100644
--- a/libcore/swf/SoundStreamHeadTag.cpp
+++ b/libcore/swf/SoundStreamHeadTag.cpp
@@ -144,12 +144,6 @@ SoundStreamHeadTag::loader(SWFStream& in, TagType tag, movie_definition& m,
         log_unimpl("SOUNDSTREAMHEAD contains %d unparsed bytes", endTag-curPos);
     }

-    IF_VERBOSE_PARSE(
-        log_parse(_("sound stream head: format=%s, rate=%d, 16=%d, "
-            "stereo=%d, ct=%d, latency=%d"), format, streamSoundRate,
-            +streamSound16bit, +streamSoundStereo, +sampleCount, +latency);
-    );
-
     // Store all the data in a SoundInfo object
     const media::SoundInfo sinfo(format, streamSoundStereo,
                 streamSoundRate, sampleCount, streamSound16bit, latency);
diff --git a/libcore/swf/tag_loaders.cpp b/libcore/swf/tag_loaders.cpp
index c0da0ce..ed14a1e 100644
--- a/libcore/swf/tag_loaders.cpp
+++ b/libcore/swf/tag_loaders.cpp
@@ -186,12 +186,6 @@ define_sound_loader(SWFStream& in, TagType tag, movie_definition& m,
         delaySeek = in.read_s16();
     }

-    IF_VERBOSE_PARSE(
-        log_parse(_("define sound: ch=%d, format=%s, rate=%d, 16=%d, "
-            "stereo=%d, ct=%d, delay=%d"), id, format, sample_rate,
-            sample_16bit, stereo, sample_count, delaySeek);
-    );
-
     // If we have a sound_handler, ask it to init this sound.

     if (handler) {


Note: The video counterpart of this enum (media::videoCodecType) is also used in libcore's SWF parser at `libcore/swf/DefineVideoStreamTag.cpp:89` but the code didn't try to print out the codec name, so it didn't cause any more link error.

Gnash: 0.8.11dev (patched against git 484ca4c 29-Mar-2016)
System: Debian GNU/Linux 7.0 Wheezy i386

Nutchanon Wetchasit <nachanon>
Tue 10 May 2016 02:52:03 PM UTC, comment #6: 

For the compiler warning, thanks for pointing that out; definitely a good idea to include it in the fix since it is related to `USE_MEDIA` code exclusion. Maybe I didn't stumble on it because it didn't cause an immediate build problem.

But for the operator issue: If the MediaParser parts of the first patch are omitted, the build results in link error on my machine:

../libcore/.libs/libgnashcore.so: undefined reference to `gnash::media::operator<<(std::ostream&, gnash::media::audioCodecType const&)'
collect2: error: ld returned 1 exit status
make[4]: *** [gtk-gnash] Error 1
make[4]: Leaving directory `/home/window/prog/gnash.git/gui'
make[3]: *** [all-recursive] Error 1
make[3]: Leaving directory `/home/window/prog/gnash.git/gui'
make[2]: *** [all] Error 2
make[2]: Leaving directory `/home/window/prog/gnash.git/gui'
make[1]: *** [all-recursive] Error 1
make[1]: Leaving directory `/home/window/prog/gnash.git'
make: *** [all] Error 2


The linker command in my case (run in `gui` subfolder) was:

ccache g++ \
        -g -O2 -std=c++11 -W -Wall -Wcast-qual -Wpointer-arith -Wreturn-type -Wnon-virtual-dtor -Wunused \
        -fvisibility-inlines-hidden -fvisibility=hidden -fpie -fPIE -Wformat -Werror=format-security \
        -fstack-protector --param ssp-buffer-size=4 \
        -Wl,--as-needed -fPIE -pie \
        -Wl,-z -Wl,relro -Wl,-z -Wl,now \
        -o .libs/gtk-gnash \
        gtk_gnash-gnash.o gtk_gnash-gui.o gtk_gnash-Player.o gtk_gnash-NullGui.o gtk_gnash-ScreenShotter.o gtk_gnash-gtk_canvas.o gtk_gnash-gtk.o gtk_gnash-gui_gtk.o gtk_gnash-gtk_glue_agg.o \
        -Wl,--export-dynamic \
        ../libcore/.libs/libgnashcore.so ../libdevice/.libs/libgnashdevice.so ../librender/.libs/libgnashrender.so ../libbase/.libs/libgnashbase.so ../libsound/.libs/libgnashsound.so \
        -lgtk-x11-2.0 -lgdk-x11-2.0 -lgio-2.0 -lpangoft2-1.0 -lpangocairo-1.0 -lgdk_pixbuf-2.0 \
        /usr/lib/i386-linux-gnu/libcairo.so /usr/lib/i386-linux-gnu/libfreetype.so \
        -lfontconfig -lpangox-1.0 -lpango-1.0 -latk-1.0 -lgobject-2.0 -lglib-2.0 \
        /usr/lib/i386-linux-gnu/libexpat.so \
        -L/usr/lib/i386-linux-gnu \
        -lX11 -lXinerama -lXext -lSM -lICE -lboost_program_options-mt -lagg_pic -lrt \
        -Wl,-rpath -Wl,/usr/local/lib/gnash


I have tried listing all `media::operator` symbol in the build tree to find potential implementation and found none; all of them are unresolved ("U") symbol:

$ find -name '*.so' -o -name '*.o' | xargs -d \\n -L 1 sh -c 'nm -gC $0 | xargs -L 1 -d \\n echo $0:' | grep media::operator
./libcore/.libs/libgnashcore.so:          U gnash::media::operator<<(std::ostream&, gnash::media::audioCodecType const&)
./libcore/.libs/tag_loaders.o:          U gnash::media::operator<<(std::ostream&, gnash::media::audioCodecType const&)
./libcore/.libs/libgnashcore-0.8.11dev.so:          U gnash::media::operator<<(std::ostream&, gnash::media::audioCodecType const&)
./libcore/.libs/SoundStreamHeadTag.o:          U gnash::media::operator<<(std::ostream&, gnash::media::audioCodecType const&)
$


From the command output, it looks like that `ostream << media::audioCodecType` is implmented only in libgnashmedia (which is unavailable when using `--disable-media` build flag).

Gnash: 0.8.11dev (patched against git 484ca4c 29-Mar-2016)
System: Debian GNU/Linux 7.0 Wheezy i386

Nutchanon Wetchasit <nachanon>
Mon 09 May 2016 12:07:04 PM UTC, comment #5: 

Another issue is this warning:

/usr/src/gnash/gnash-head/libcore/asobj/NetStream_as.cpp:198:1: warning: no return statement in function returning non-void [-Wreturn-type]
 }
 ^
which can be fixed with this:

diff --git a/libcore/asobj/NetStream_as.cpp b/libcore/asobj/NetStream_as.cpp
index d500ea0..06dec58 100644
--- a/libcore/asobj/NetStream_as.cpp
+++ b/libcore/asobj/NetStream_as.cpp
@@ -194,6 +194,8 @@ NetStream_as::bufferLength()
 #ifdef USE_MEDIA
     if (_parser.get() == nullptr) return 0;
     return _parser->getBufferLength();
+#else
+    return 0;
 #endif  // USE_MEDIA
 }


I checked, and the output operator could probably just remain in MediaParser.cpp, why did you feel the need to move it ?

Sandro Santilli <strk>
Group Member
Mon 09 May 2016 10:47:53 AM UTC, comment #4: 

The move of output operator for audioCodecType from .cpp to .h file would change ABI, is that necessary ? (in 0001_fix-disable-media-compile-link-failure.patch)

Maybe there's another implementation file that's built anyway ?

Sandro Santilli <strk>
Group Member
Mon 09 May 2016 03:05:30 AM UTC, comment #3: 

I have made a variant of patch 2 which fixes the crash on Gnash 0.8.10. (Patch 1 is not required for 0.8.10, since it builds without failure) It was tested on the same configuration as comment 2.

Patch series for Gnash 0.8.10 users:

  • 1/1: 0002_refuse-invoking-nonexistent-media-handler-0.8.10.patch (file #37106)


Patch series for current 0.8.11dev master version:

  • 1/2: 0001_fix-disable-media-compile-link-failure.patch (file #37093)
  • 2/2: 0002_refuse-invoking-nonexistent-media-handler.patch (file #37094)


Gnash: 0.8.10 (git)
System: Debian GNU/Linux 5.0 Lenny i386

Nutchanon Wetchasit <nachanon>
Sat 07 May 2016 03:11:20 AM UTC, comment #2: 

The posted patches are tested with following scenarios:

  • SWF with embeded event sound (PCM 44100 Hz 16-bit Mono, 1.0 sec)
  • SWF with embeded frame-synchronized stream sound (MP3 22050 Hz Stereo 64kbps, 10.0 sec)
  • SWF playing external event MP3 sound via `Sound.loadSound()` (MP3 44100 Hz Mono 32kbps CBR, 4.8 sec)
  • SWF playing external streaming MP3 sound loaded via `Sound.loadSound()` (MP3 44100 Hz Mono 32kbps CBR, 4.8 sec)
  • SWF with embeded frame-synchronized video (640x480 24fps FLV1 video, 44100 Hz Mono 32kbps MP3 audio, 10.0 sec)
  • SWF playing external streaming video via `NetStream.play()` (640x480 24fps 128kbps FLV1 video, 44100 Hz Mono 32kbps CBR MP3 audio, FLV container, 10.0 sec)


Gnash: 0.8.11dev (patched against git 484ca4c 29-Mar-2016)
System: Debian GNU/Linux 7.0 Wheezy i386

Nutchanon Wetchasit <nachanon>
Sat 07 May 2016 03:09:35 AM UTC, comment #1: 

Patch 2 of 2:
See the attached `0002_refuse-invoking-nonexistent-media-handler.patch`.

Gnash (at least since 0.8.10) crashes if SWF tries to play embeded event/stream sound when the build was configured with `--disable-media`. This was due to libsound not checking for possibility that there could be no media handler configured.

The patch rectify this problem by adding availablity check of `sound_handler::_mediaHandler` just before `sound_handler` attempt to start an embeded event sound or frame-sync'd stream sound.

If media handler is not configured (`sound_handler::_mediaHandler` is `NULL`), user will be presented with following messages instead of crashing:

- When SWF is trying to play an event sound:

ERROR: Could not start event sound: No media handler available


- When SWF is trying to play a frame-sync'd stream sound (message will be emitted on every frame):

ERROR: Could not start streaming sound: No media handler available


This fixes the crashing part of bug #27952.

It might need review as I'm not sure whether these should be added as compile-time checks or run-time checks. This patch uses run-time checks as it looks cleaner.

Gnash: 0.8.11dev (patched against git 484ca4c 29-Mar-2016)
System: Debian GNU/Linux 7.0 Wheezy i386


(file #37094)

Nutchanon Wetchasit <nachanon>
Sat 07 May 2016 02:59:36 AM UTC, original submission:  

Patch 1 of 2:
See the attached `0001_fix-disable-media-compile-link-failure.patch`.

Current development version of Gnash does not compile when user configured the build with `./configure --disable-media`, due to multiple issues in libcore and libsound.

This patch does following things to rectify problems:


This fixes the build failure part of bug #27952.

It might need review as I'm not really familiar with M4/Autotools, and I'm not sure if there's a better way to fix SWF parser's linkage than simply inlining MediaParser's operator function.

Gnash: 0.8.11dev (patched against git 484ca4c 29-Mar-2016)
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 #37120:  0001_fix-disable-media-compile-link-failure-v2.patch added by nachanon (6KiB - application/octet-stream - Patch to fix compilation and link failure on a build configured with `--disable-media`, with no libmedia change)
file #37106:  0002_refuse-invoking-nonexistent-media-handler-0.8.10.patch added by nachanon (1KiB - application/octet-stream - Patch to fix crash when tried to play SWF sound on 0.8.10 build configured with `--disable-media`)
file #37094:  0002_refuse-invoking-nonexistent-media-handler.patch added by nachanon (2KiB - application/octet-stream - Patch to fix crash when tried to play SWF sound on a build configured with `--disable-media`)

 

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 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 8 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2016-05-11 strk StatusReady For Test Done
        Open/ClosedOpen Closed
    2016-05-11 strk StatusNone Ready For Test
        Assigned toNone strk
    2016-05-10 nachanon Attached File- Added 0001_fix-disable-media-compile-link-failure-v2.patch, #37120
    2016-05-09 nachanon Attached File- Added 0002_refuse-invoking-nonexistent-media-handler-0.8.10.patch, #37106
    2016-05-07 nachanon Attached File- Added 0002_refuse-invoking-nonexistent-media-handler.patch, #37094
    2016-05-07 nachanon Attached File- Added 0001_fix-disable-media-compile-link-failure.patch, #37093

    Back to the top

    Powered by Savane 3.13-02a9.
    Corresponding source code