After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 738237 - baseparse: parser is never marked as FLAG_LOST_SYNC on discont
baseparse: parser is never marked as FLAG_LOST_SYNC on discont
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gstreamer (core)
1.4.3
Other All
: Normal minor
: 1.5.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2014-10-09 14:27 UTC by Nicolas Huet
Modified: 2015-08-15 11:00 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
patch fixing the issue (1.38 KB, patch)
2014-10-09 14:31 UTC, Nicolas Huet
none Details | Review
base: fix GST_BASE_PARSE_FLAG_LOST_SYNC (3.09 KB, patch)
2015-04-28 15:06 UTC, Ilya Konstantinov
none Details | Review
base: fix GST_BASE_PARSE_FLAG_LOST_SYNC (w/update_flags, update_frame) (3.05 KB, patch)
2015-04-28 15:19 UTC, Ilya Konstantinov
committed Details | Review
gstreamer 1.4.5 log (17.12 KB, text/x-log)
2015-08-14 21:42 UTC, betacentauri
  Details
gstreamer 1.5.1 log (152.00 KB, text/x-log)
2015-08-14 21:45 UTC, betacentauri
  Details

Description Nicolas Huet 2014-10-09 14:27:34 UTC
In the function gst_base_parse_prepare_frame, discont is set to FALSE before creating the frame.

Then the frame is created and the flags are applied to the frame calling the function gst_base_parse_frame_update
As discont is already FALSE, the frame is not marked as GST_BASE_PARSE_FLAG_LOST_SYNC
Comment 1 Nicolas Huet 2014-10-09 14:31:28 UTC
Created attachment 288135 [details] [review]
patch fixing the issue
Comment 2 Thiago Sousa Santos 2015-04-28 14:15:07 UTC
Review of attachment 288135 [details] [review]:

The fix is correct but perhaps we should leave a note for future people reading this code so that they are aware that _update_frame() needs to come before resetting the discont.

Or maybe there is some other option to make it more readable that I couldn't think about.
Comment 3 Ilya Konstantinov 2015-04-28 14:35:30 UTC
Options:
a) gst_base_parse_frame_update is static inline and only used in gst_base_parse_prepare_frame. Perhaps bringing it inside will help clarity.

b) bring this section inside gst_base_parse_frame_update:

  if (parse->priv->discont) {
    GST_DEBUG_OBJECT (parse, "marking DISCONT");
    GST_BUFFER_FLAG_SET (buffer, GST_BUFFER_FLAG_DISCONT);
    parse->priv->discont = FALSE;
  }

c) move "parse->priv->discont = FALSE;" to the end of the function, to clarify that clearing it is separate from handling it
Comment 4 Ilya Konstantinov 2015-04-28 15:06:42 UTC
Created attachment 302520 [details] [review]
base: fix GST_BASE_PARSE_FLAG_LOST_SYNC

This version tests and clears flags in gst_base_parse_frame_update.

Following flags (and not-flags) are cleared:

parse->priv->discont = FALSE;
parse->priv->new_frame = FALSE;
frame->offset = parse->priv->prev_offset = parse->priv->offset;

I wonder if just doing it one function is more clear.
Comment 5 Ilya Konstantinov 2015-04-28 15:19:04 UTC
Created attachment 302522 [details] [review]
base: fix GST_BASE_PARSE_FLAG_LOST_SYNC (w/update_flags, update_frame)

Another variant, splitting into:

* gst_base_parse_update_flags
* gst_base_parse_update_frame
Comment 6 Thiago Sousa Santos 2015-04-28 15:58:56 UTC
Thanks for the patch.

commit b58245ac0a53336ea90e6462a0a3ca3d70e3d8d0
Author: Ilya Konstantinov <ilya.konstantinov@gmail.com>
Date:   Tue Apr 28 17:54:51 2015 +0300

    baseparse: fix GST_BASE_PARSE_FLAG_LOST_SYNC
    
    Since frame->priv->discont was cleared earlier,
    GST_BASE_PARSE_FLAG_LOST_SYNC was never being set.
    
    Take the chance to refactor the frame creation a bit to
    organize the flags setting and reset.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=738237
Comment 7 Tim-Philipp Müller 2015-05-08 10:35:43 UTC
This seems to have broken the wavpackparse unit test in -good.
Comment 8 Ilya Konstantinov 2015-05-08 13:49:07 UTC
Confirming; before patch:

Running suite(s): wavpackparse
100%: Checks: 2, Failures: 0, Errors: 0

After patch:

Running suite(s): wavpackparse
50%: Checks: 2, Failures: 1, Errors: 0
elements/wavpackparse.c:183:F:general:test_parsing_invalid_first_header:0: 'num_buffers' (0) is not equal to '1' (1)
Running suite(s): wavpackparse
50%: Checks: 2, Failures: 1, Errors: 0
elements/wavpackparse.c:183:F:general:test_parsing_invalid_first_header:0: 'num_buffers' (0) is not equal to '1' (1)

Will look into it.
Comment 9 Ilya Konstantinov 2015-05-08 23:13:00 UTC
SOURCE CODE ARCHEOLOGY FOLLOWS:

The concept of 'sync' was introduced in commit 1bc8301c795ea5e4bb3389c2fc9a5ea4186adc18 (2009-10-28) by Mark Nauwelaerts. It was defined as simply:

  gboolean gst_base_parse_get_sync (GstBaseParse * parse) {
    ...
    /* losing sync is pretty much a discont (and vice versa), no ? */
    ret = !parse->priv->discont;
    ...
  }

In this patch, Mark also set 'discont' to be initially TRUE, meaning the element starts in the state of not-synced.

At a latter point, sync was replaced with LOST_SYNC (i.e. inverted). This made the semantics murkier, since discont is TRUE initially, however no sync was ever *LOST*.

Finally, Mark himself introduced the bug in b761f5479a2ca977c07d70be39f1f0eb764b034e (2012-02-13), making his very own LOST_SYNC meaningless, since it was no longer ever getting set.

The initial revision of wavpackparse.c is from 2012-02-28, two weeks later, also implemented by Mark Nauwelaerts. The initial revision of wavpackparse.c contains:

  lost_sync = GST_BASE_PARSE_LOST_SYNC (parse);

but following the 2012-02-13, the LOST_SYNC flag was never set, and the lost_sync codepath was never tested.
Comment 10 Ilya Konstantinov 2015-05-08 23:15:44 UTC
Following the above, can someone (Mark? Tim?) explain what lost_sync is SUPPOSED to be?
Comment 11 Mark Nauwelaerts 2015-05-10 07:58:24 UTC
Afaik it should still hold lost_sync == !(in sync) == discont.  As such, it should probably be set on start, and whenever the subclass decides to skip some bytes (representing a discontinuity).

There may well have been some glitching causing it not to be set properly.  Unfortunately, baseparse (code) is not always the clearest and cleanest, and it could well do with some re-writing (and tweaked ABI/semantics), but that's another one ...

So, afaics, the patch looks ok, and so does wavpackparse, and there's reasonable chance there is some problem with the unit test.
Comment 12 Ilya Konstantinov 2015-05-10 11:02:48 UTC
(In reply to Mark Nauwelaerts from comment #11)
> So, afaics, the patch looks ok, and so does wavpackparse, and there's
> reasonable chance there is some problem with the unit test.

It might just be that wavpackparse code is correct, but then - how did it work since 2012? Cause, since 2012, lost_sync was ALWAYS unset.
Comment 13 Mark Nauwelaerts 2015-05-10 12:25:15 UTC
lost_sync is not essential to wavpackparse's operation (as it is to most parsers, iirc).  In any case, following commit fixes the unit test:

commit 692df969ea5fbdf033f20d9ad8c763f7b666f3c2
Author: Mark Nauwelaerts <mnauw@users.sourceforge.net>
Date:   Sun May 10 14:21:04 2015 +0200

    tests: wavpackparse: fix unit test
    
    See also https://bugzilla.gnome.org/show_bug.cgi?id=738237


[indeed, that's a most unhelpful commit message :-(, but anything more would feel like repeating the commit content ;-) ]
Comment 14 betacentauri 2015-08-14 21:27:34 UTC
With applied patch I cannot playback some mkv files.
Comment 15 betacentauri 2015-08-14 21:42:04 UTC
Created attachment 309310 [details]
gstreamer 1.4.5 log

Log with gstreamer 1.4.5. mkv playback works.
Comment 16 betacentauri 2015-08-14 21:45:26 UTC
Created attachment 309311 [details]
gstreamer 1.5.1 log

With gstreamer 1.5.1 some mkv files cannot be played.
I bisected to find the problematic commit. Without the "baseparse: fix GST_BASE_PARSE_FLAG_LOST_SYNC" patch it works. With it it don't works.
Comment 17 Sebastian Dröge (slomo) 2015-08-15 07:27:15 UTC
can you provide a sample file?
Comment 18 betacentauri 2015-08-15 09:51:36 UTC
http://www.file-upload.net/download-10841985/test-002.mkv.html

I hope you can confirm the bug.
We use here gstreamer 1.5.1 on enigma2 box.
Comment 19 Sebastian Dröge (slomo) 2015-08-15 10:38:15 UTC
There are always 716 bytes between the end of one DTS frame and the start of the next. So we never find two DTS sync codes right after another, and never get of the "finding sync" mode.

Before this patch here, it never checked for two consecutive DTS sync codes.
Comment 20 Sebastian Dröge (slomo) 2015-08-15 10:52:38 UTC
ffmpeg also detects the framesize as 2012 and seems to skip over that data... so let's just never look for a second sync code?
Comment 21 Sebastian Dröge (slomo) 2015-08-15 11:00:31 UTC
commit 64b06d1829493d9119022ed5e8476f3e9b486ec5
Author: Sebastian Dröge <sebastian@centricular.com>
Date:   Sat Aug 15 12:58:50 2015 +0200

    dcaparse: Don't look for a second syncword
    
    There are streams out there that consistently contain garbage between
    every frame so we never ever find a second consecutive syncword.
    
    See https://bugzilla.gnome.org/show_bug.cgi?id=738237