Created attachment 288135 [details] [review]
patch fixing the issue

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.

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

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.

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

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

This seems to have broken the wavpackparse unit test in -good.

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.

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.

Following the above, can someone (Mark? Tim?) explain what lost_sync is SUPPOSED to be?

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.

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

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 ;-) ]

With applied patch I cannot playback some mkv files.

Created attachment 309310 [details]
gstreamer 1.4.5 log

Log with gstreamer 1.4.5. mkv playback works.

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.

can you provide a sample file?

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.

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.

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?

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