GNOME Bugzilla – Bug 749690
splitfilesrc: Implement binary search in find_part_for_offset
Last modified: 2015-05-26 16:44:10 UTC
Attached patch implements binary search using gstutils in find_part_for_offset function.
Created attachment 303773 [details] [review] splitfilesrc: Implement binary search in find_part_for_offset
Review of attachment 303773 [details] [review]: ::: gst/multifile/gstsplitfilesrc.c @@ +374,3 @@ +{ + if (part->start > *offset && part->stop < *offset) + return -1; offset can't be both <= start and >= stop for a file part. The match function should look like: if (*offset > part->stop) return -1; /* The target is after this part */ if (*offset < part->start) return 1; /* The target is before this part */ return 0; /* This is the target part */
(In reply to Jan Schmidt from comment #2) > Review of attachment 303773 [details] [review] [review]: > > ::: gst/multifile/gstsplitfilesrc.c > @@ +374,3 @@ > +{ > + if (part->start > *offset && part->stop < *offset) > + return -1; > > offset can't be both <= start and >= stop for a file part. > > The match function should look like: > > if (*offset > part->stop) > return -1; /* The target is after this part */ > if (*offset < part->start) > return 1; /* The target is before this part */ > return 0; /* This is the target part */ Thanks for your review. I'll update new patch to include your comment.
Created attachment 303829 [details] [review] splitfilesrc: Implement binary search in find_part_for_offset
Review of attachment 303829 [details] [review]: Thanks for the patch - still needs one minor fix I missed the first time. ::: gst/multifile/gstsplitfilesrc.c @@ +390,3 @@ + part = + gst_util_array_binary_search (src->parts, src->num_parts, + sizeof (GstFilePart), Sorry - I missed this on the first review. The size of the array elements is one pointer, not an entire GstFilePart - so it should be 'sizeof (GstFilePart *)' not 'sizeof (GstFilePart)'. I imagine this would crash fairly badly - have you tested it at all?
(In reply to Jan Schmidt from comment #5) > Review of attachment 303829 [details] [review] [review]: > > Thanks for the patch - still needs one minor fix I missed the first time. > > ::: gst/multifile/gstsplitfilesrc.c > @@ +390,3 @@ > + part = > + gst_util_array_binary_search (src->parts, src->num_parts, > + sizeof (GstFilePart), > > Sorry - I missed this on the first review. The size of the array elements is > one pointer, not an entire GstFilePart - so it should be 'sizeof > (GstFilePart *)' not 'sizeof (GstFilePart)'. > > I imagine this would crash fairly badly - have you tested it at all? Thanks for the review. I tested using gst-launch like below command. gst-launch-1.0 playbin uri="splitfile:///home/yongjinohn/testmovie/Gossip1x01_1.avi" It's working for me. Is it right about the sizeof (GstFilePart *)? I just reference qtdemux code. Here is the qtdemux code. 969 QtDemuxSample *result; 970 guint32 index; 971 972 /* convert media_time to mov format */ 973 media_time = 974 gst_util_uint64_scale_ceil (media_time, str->timescale, GST_SECOND); 975 976 result = gst_util_array_binary_search (str->samples, str->stbl_index + 1, 977 sizeof (QtDemuxSample), (GCompareDataFunc) find_func, 978 GST_SEARCH_MODE_BEFORE, &media_time, NULL);
Created attachment 303876 [details] Dot-graph In addition, I attach the dot graph for splitfilesrc.
QtDemux is storing an array of actual QtDemuxSamples directly, so sizeof(QtDemuxSample) is correct there. splitfilesrc is storing an array of pointers to GstFileParts - so sizeof (GstFilePart *) If you're just testing with one file, you'll never exercise that code. It only runs when switching to a different file, so you need at least 2.
(In reply to Jan Schmidt from comment #8) > QtDemux is storing an array of actual QtDemuxSamples directly, so > sizeof(QtDemuxSample) is correct there. > > splitfilesrc is storing an array of pointers to GstFileParts - so sizeof > (GstFilePart *) > > If you're just testing with one file, you'll never exercise that code. It > only runs when switching to a different file, so you need at least 2. I misunderstood your instructions. I'll modify source code for your comment. I have a question. I just use this command for split some avi file. is it right? "avisplit -s 3 -i filename" 3 means 3 megabyte. This plays the first file fine and then stops:( Also, splitfilesrc plays the first file and then stops without this patch.
You can do this: $ split -b 1M some.file > somefile- to split any file into chunks of 1MB. And then use something like this: $ gst-play-1.0 'splitfile:///path/to/somefile-*' or (if not up-to-date git master): $ gst-play-1.0 --interactive 'splitfile:///path/to/somefile-*' In any case, when proposing a patch please make sure that you test the code path(s) you modify.
@Tim Thanks for your comment:) I misunderstood in this test case. Don't worry about that. I always make sure proposing patch before upload to bugzilla.(In reply to Tim-Philipp Müller from comment #10) > You can do this: > > $ split -b 1M some.file > somefile- > > to split any file into chunks of 1MB. > > And then use something like this: > > $ gst-play-1.0 'splitfile:///path/to/somefile-*' > > or (if not up-to-date git master): > > $ gst-play-1.0 --interactive 'splitfile:///path/to/somefile-*' > > In any case, when proposing a patch please make sure that you test the code > path(s) you modify. @Tim Thanks for your comment:) I misunderstood in this test case. Don't worry about that. I always make sure proposing patch before upload to bugzilla.
(In reply to Jan Schmidt from comment #8) > QtDemux is storing an array of actual QtDemuxSamples directly, so > sizeof(QtDemuxSample) is correct there. > > splitfilesrc is storing an array of pointers to GstFileParts - so sizeof > (GstFilePart *) > > If you're just testing with one file, you'll never exercise that code. It > only runs when switching to a different file, so you need at least 2. @Jan Schmidt If I change from GstFilePart to GstFilePart *, It's not working. Here is the error message. [jhbuild] yongjinohn@ubuntu:~/testmovie$ gst-launch-1.0 playbin uri="splitfile:///home/yongjinohn/testmovie/Gossip1x01_*" Setting pipeline to PAUSED ... Pipeline is PREROLLING ... ERROR: from element /GstPlayBin:playbin0/GstURIDecodeBin:uridecodebin0/GstSplitFileSrc:source: Could not perform seek on resource. Additional debug info: gstsplitfilesrc.c(514): gst_split_file_src_create (): /GstPlayBin:playbin0/GstURIDecodeBin:uridecodebin0/GstSplitFileSrc:source: Seek to 18446744073698017280 in /home/yongjinohn/testmovie/Gossip1x01_al failed ERROR: pipeline doesn't want to preroll. Setting pipeline to NULL ... Freeing pipeline ... If we use GstFilePart type, It's working for me.
Also I tested using gst-play-1.0 'splitfile:///home/yongjinohn/testmovie/Gossip1x01_*' for testing but It's same result.
Sorry - you're right. The parts array is an array of GstFilePart, so sizeof (GstFilePart) is right - I misread the code. Your latest patch works fine for me in testing - pushed.
(In reply to Jan Schmidt from comment #14) > Sorry - you're right. The parts array is an array of GstFilePart, so sizeof > (GstFilePart) is right - I misread the code. > > Your latest patch works fine for me in testing - pushed. Thanks for the review and push my patch.:)