GNOME Bugzilla – Bug 722705
Factor out common init/reset code from matroska parse/demux
Last modified: 2014-02-11 20:53:40 UTC
While working on #722311 I noticed a fairly sized amount of repeated utility read-context init/reset code on these two elements. Attached patch factors out the duplication shoving some 100 LOCs off the two elements (combined).
Created attachment 266890 [details] [review] factor out common demux/parse read-context setup/reset code
Review of attachment 266890 [details] [review]: ::: gst/matroska/matroska-demux.c @@ +190,3 @@ GstMatroskaDemux *demux = GST_MATROSKA_DEMUX (object); + gst_matroska_read_common_finalize_ctx (&demux->common); It's called GstMatroskaReadCommon, not GstMatroskaReadCommonCtx @@ -197,3 @@ - if (demux->common.global_tags) { - gst_tag_list_unref (demux->common.global_tags); - demux->common.global_tags = NULL; I think the tags and the ptr array should be emptied in reset() already but that's not a bug in your patch ::: gst/matroska/matroska-read-common.c @@ +2859,3 @@ + +void +gst_matroska_read_common_reset_ctx (GstElement * element, I assume you made sure you copied everything over from the other two reset functions here? At least the index stuff is missing, but that can and should disappear anyway.
(In reply to comment #2) > Review of attachment 266890 [details] [review]: > > ::: gst/matroska/matroska-demux.c > @@ +190,3 @@ > GstMatroskaDemux *demux = GST_MATROSKA_DEMUX (object); > > + gst_matroska_read_common_finalize_ctx (&demux->common); > > It's called GstMatroskaReadCommon, not GstMatroskaReadCommonCtx Yeah, figured out _ctx made more sense for these funcs but renamed them in the latest patch. > [...] > ::: gst/matroska/matroska-read-common.c > @@ +2859,3 @@ > + > +void > +gst_matroska_read_common_reset_ctx (GstElement * element, > > I assume you made sure you copied everything over from the other two reset > functions here? At least the index stuff is missing, but that can and should > disappear anyway. You mean the element_index stuff? That code was disabled already so I just dropped it. As per the rest as far as I can see everything is there in place. Thanks for your review.
Created attachment 267679 [details] [review] factor out common demux/parse read-context setup/reset code Updated patch with the functions renamed (dropped the _ctx postfix).
Review of attachment 267679 [details] [review]: Looks good but: ::: gst/matroska/matroska-read-common.c @@ +2852,3 @@ + if (ctx->global_tags) { + gst_tag_list_unref (ctx->global_tags); + ctx->global_tags = NULL; Move this to reset() in a second commit
As reset() is creating a new list after unrefing, unless I'm missing something, I don't see a way to easily move that section around. Can you elaborate\ please? Thanks a lot for your review. Patch has been committed now and I'm closing this once we clear the above out.
Nevermind, it's all good :)
Cool, thanks for taking a look.