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 712643 - qtdemux: couple of issues with vobsub
qtdemux: couple of issues with vobsub
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
git master
Other All
: Normal normal
: 1.3.1
Assigned To: Jan Schmidt
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2013-11-19 00:41 UTC by Matej Knopp
Modified: 2014-05-26 07:45 UTC
See Also:
GNOME target: ---
GNOME version: ---



Description Matej Knopp 2013-11-19 00:41:40 UTC
First issue is that the demuxer seems to be outputting empty (2 byte) buffers, which should be ignored, they are supposed to be terminator samples.

Second issue is that palette event is not created, resulting in the subtitles not being rendered correctly.  I haven't found any specification of vobsub in MP4, but perhaps the way handbrake creates codec data might be helpful

https://github.com/HandBrake/HandBrake/blob/master/libhb/muxmp4.c
Comment 1 Matej Knopp 2013-11-19 00:46:37 UTC
Sample file
https://s3.amazonaws.com/MatejK/Samples/mp4-vobsub.m4v

There are only like 3 cues in the file, first one is several seconds in
Comment 2 Jan Schmidt 2013-11-19 09:47:25 UTC
The sample file is 403
Comment 3 Matej Knopp 2013-11-19 11:06:13 UTC
Sorry, permissions should be fixed now. Also the dimensions might not match so the subtitles might get cropped, but that's different issue.
Comment 4 Jan Schmidt 2013-11-20 15:40:51 UTC
Fixed, in qtdemux and dvdspu:

commit 81e2c8130a7a2a9316cd79d3f84d97979c7010b6
Author: Jan Schmidt <jan@centricular.com>
Date:   Thu Nov 21 02:28:27 2013 +1100

    isomp4: Handle mp4s subpicture streams better.
    
    Clean up the handling of mp4s streams. Use the generic esds
    descriptor function to extract the palette, instead of hard coding
    a wrong magic offset.
    
    Add some more size safety checks when parsing ES descriptors, and
    replace magic numbers with the descriptive constants that are already
    defined.
    
    Enhance dump output for stsd atoms.
    
    Streams from both bug 712643 and historic bug 568278 now both work
    correctly.
    
    Fixes: #712643

commit c336f7642cd83a4521a91198e6c9ccee05c84f1d
Author: Jan Schmidt <jan@centricular.com>
Date:   Wed Nov 20 12:46:23 2013 +1100

    dvdspu: Handle vobsub packets smaller than 4 bytes
    
    In particular, handle the case of an empty packet with size 0 bytes.
    
    Partially fixes: #712643
Comment 5 Matej Knopp 2013-11-24 22:02:07 UTC
I'm just wondering if the empty buffer shouldn't have been handled by the demuxer. I'd say it is a muxing details - since samples in MP4 don't have duration, an empty next sample is used to determine sample length.
Comment 6 Jan Schmidt 2013-11-25 00:25:19 UTC
Not sure it makes any difference, and handling it in the demux looked much harder - easier just to discard the packets, which the SPU should have been doing anyway.

I'm not sure why they felt compelled to add the 0 byte packets in the first place - the SPU packet contents themselves should have all the relevant timing information.
Comment 7 Matej Knopp 2013-11-25 00:52:49 UTC
My concern for the empty packets is remuxing.
Comment 8 Jan Schmidt 2013-11-25 01:25:53 UTC
OK, qtdemux changed to discard the terminator packets, after brief discussion on IRC:

commit fdfc6a2a86dfd95f2d059b1b031e694cce09f67f
Author: Jan Schmidt <jan@centricular.com>
Date:   Mon Nov 25 12:13:43 2013 +1100

    qtdemux: Discard 2 byte subpicture packets
    
    As for text subtitles and as suggested in #712643, throw
    away the 2 byte terminator packets that some encoders insert.
    
    This will make things better when remuxing and causes generation
    of gap events.
Comment 9 Matej Knopp 2013-11-25 02:46:43 UTC
Seems to work fine, except for the following 

GStreamer-Critical: void gst_buffer_unmap(GstBuffer *, GstMapInfo *): assertion `GST_IS_BUFFER (buffer)' failed

Stacktrace

  • #4 gst_buffer_unmap at /opt/avs-debug/_build/gstreamer/gst/gstbuffer.c:1549
  • #5 gstspu_vobsub_execute_event at /opt/avs-debug/_build/gst-plugins-bad/gst/dvdspu/gstspu-vobsub.c:407
  • #6 gstspu_execute_event at /opt/avs-debug/_build/gst-plugins-bad/gst/dvdspu/gstdvdspu.c:764
  • #7 gst_dvd_spu_advance_spu at /opt/avs-debug/_build/gst-plugins-bad/gst/dvdspu/gstdvdspu.c:790
  • #8 dvdspu_handle_vid_buffer at /opt/avs-debug/_build/gst-plugins-bad/gst/dvdspu/gstdvdspu.c:617
  • #9 gst_dvd_spu_video_chain at /opt/avs-debug/_build/gst-plugins-bad/gst/dvdspu/gstdvdspu.c:566


gboolean
gstspu_vobsub_execute_event (GstDVDSpu * dvdspu)
{
  ...
  ...
    gst_dvd_spu_finish_spu_buf (dvdspu);
    ret = FALSE;
  }
>>gst_buffer_unmap (state->vobsub.buf, &map);

  return ret;
}
Comment 10 Jan Schmidt 2013-11-25 04:06:57 UTC
That looks like a local change of your own. git master has that code like this:

  if (next_blk != state->vobsub.cur_cmd_blk) {
    /* Advance to the next block of commands */
    gst_dvd_spu_setup_cmd_blk (dvdspu, next_blk, start, end);
  } else {
    /* Next Block points to the current block, so we're finished with this
     * SPU buffer */
    gst_buffer_unmap (state->vobsub.buf, &map);
    gst_dvd_spu_finish_spu_buf (dvdspu);
    return FALSE;
  }
  gst_buffer_unmap (state->vobsub.buf, &map);
Comment 11 Matej Knopp 2013-11-25 04:15:07 UTC
Sorry, looks like it was broken by this commit

http://cgit.freedesktop.org/gstreamer/gst-plugins-bad/commit/?id=db915754c15009dfe61bbc016b801f5bb63b1e84
Comment 12 Sebastian Dröge (slomo) 2014-05-26 07:45:46 UTC
Matej, is there a problem still with latest GIT?