Created attachment 312034 [details] [review]
add conceal property for 1.0

Same patch for the 1.0 port (https://bugzilla.gnome.org/show_bug.cgi?id=733819).

The commented "gst_teletextdec_update (teletext);" function is from my cache navigation patch (https://bugzilla.gnome.org/show_bug.cgi?id=666247) and should be enabled when the other patch was accepted.

I don't know how to handle this the right way...

Any news here?

Should I create a new diff against current git version?

Comment on attachment 312033 [details] [review]
add conceal property

For 0.10, no longer relevant

Review of attachment 312034 [details] [review]:

::: ext/teletextdec/gstteletextdec.c
@@ +366,3 @@
+    case PROP_CONCEAL:
+      teletext->concealed = g_value_get_boolean (value);
+//      gst_teletextdec_update (teletext);

Why is this commented out? (Also don't use C++/C99 comments)

Should update() be called on the next opportunity here?

Created attachment 326239 [details] [review]
add property to show/hide hidden content

patch against current git, run gst-indent and remove C++ comment

Hi Sebastian,

thanks for the review.

It's commented out because I don't know how to handle it correct (see comment #1).

After changing the concealed flag the page should be rendered again to make the changes visible instantly.

Maybe there is a simpler/better way for doing this, but I'm an gstreamer noob... ;)

Thomas

You could remember it in a flag and next time the chain function is called you would re-render instead of outputting the cached version. Or is the problem here that you won't get any new buffers for a long time?

As I remember right without the update function it was only refreshed when the pagenumber was received again - and this can take several seconds.

So the user clicks to show hidden content and nothing happens. That's the reason you need the page update without delay.

You could also check for the need to update whenever a video frame arrives. That should happen more often.

Ok, but what is the problem to refresh the page once when the user clicks?

The commented out function simply does the following:

void gst_teletextdec_update (GstTeletextDec * teletext)
{
  page_info *pi = g_new (page_info, 1);
  pi->pgno = teletext->pageno;
  pi->subno = teletext->subno == -1 ? VBI_ANY_SUBNO : teletext->subno;

  g_mutex_lock (&teletext->queue_lock);
  g_queue_push_tail (teletext->queue, pi);
  g_mutex_unlock (&teletext->queue_lock);

  gst_teletextdec_push_page (teletext);
}

I don't know if this is the correct way to refresh, but it was working fine here (together with the navigation patch mentioned above).

push_page() sounds like it is pushing data into the pipeline. You don't really want to do that from the application thread (that is setting the property), as it can block the application for quite a while and in general data flow must only happen from streaming threads.

I've no idea how to handle the update stuff correctly, sorry.

Is the other ok (removing the commented out line) to be checked in?

We would have an delay but better as nothing. Maybe somebody else knows how to force a clean page update...

-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/gstreamer/gst-plugins-bad/issues/301.