GNOME Bugzilla – Bug 777825
isoff: Move isoff to gst-libs
Last modified: 2017-08-26 01:12:19 UTC
ISOBMFF is used for DASH/MSS and even HLS latest version. Now, DASH and MSS have their own parsing function even though qtdemux is there. And although HLS does not have the isobmff parser yet, it might be needed to extract timestamp and etc.
Created attachment 344396 [details] [review] isoff: Move isoff to gst-libs
Created attachment 344397 [details] [review] tests: Rename dash_isoff to isoff
If we do that, we should also port the MSS code to use this.
(In reply to Sebastian Dröge (slomo) from comment #3) > If we do that, we should also port the MSS code to use this. Sure, I'd like to do it also :) I'm working for it now, but I'd like other people's opinions on this, before going further.
I think it's a good idea, we shouldn't duplicate all this code.
(In reply to Sebastian Dröge (slomo) from comment #5) > I think it's a good idea, we shouldn't duplicate all this code. Thank you :) I'll update patch, soon.
Created attachment 344473 [details] [review] isoff: Move isoff to gst-libs
Created attachment 344474 [details] [review] tests: Rename dash_isoff to isoff
Created attachment 344475 [details] [review] isoff: Rename GstIsoffParserResult to GstSidxParserResult also GST_ISOFF_PARSER_{OK,DONE,UNEXPECTED,ERROR} to GST_SIDX_PARSER_{}
Created attachment 344476 [details] [review] isoff: Parse moov box To figure out track_id, timescale, and handler_type
Created attachment 344477 [details] [review] isoff: Parse tfdt box To figure out the first timestamp of a fragment
Created attachment 344478 [details] [review] tests: isoff: Add test cases for parsing moov/tfdt box
Created attachment 344479 [details] [review] isoff: Parse tfxd and tfrf box This code is imported from mssdemux's tfxd/tfrf parsing function
Created attachment 344480 [details] [review] tests: isoff: Add a test case for parsing tfrf/tfxd box
Created attachment 344481 [details] [review] smoothstreaming: Use isoff to parsing tfxd/tfrf
(In reply to Seungha Yang from comment #15) > Created attachment 344481 [details] [review] [review] > smoothstreaming: Use isoff to parsing tfxd/tfrf This patch was verified using following live manifest http://profficialsite.origin.mediaservices.windows.net/9cc5e871-68ec-42c2-9fc7-fda95521f17d/dayoneplayready.ism/manifest (http://playready.azurewebsites.net/Home/AdaptiveTests#Live_Samples) Note that, since I have no available decrypt element, just streaming and parsing part in mssdemux were verified. Now, it's hard to find public and clear mss live url...
Any chance you can rebase these on top of master? Thank you
(In reply to Reynaldo H. Verdejo Pinochet from comment #17) > Any chance you can rebase these on top of master? Thank you OK, I'll do it soon :)
(In reply to Seungha Yang from comment #18) > (In reply to Reynaldo H. Verdejo Pinochet from comment #17) > > Any chance you can rebase these on top of master? Thank you > > OK, I'll do it soon :) Perfect. I'm interested on this landing
Created attachment 352026 [details] [review] [1/4] isoff: Move isoff to gst-libs Also rename unit test dash_isoff to isoff
Created attachment 352027 [details] [review] [2/4] isoff: Add parsing moov and tfdt To extract isobmff level timestamp, moov and tfdt parsing is required.
Created attachment 352028 [details] [review] [3/4] isoff: Add parsing mss specific tfrf and tfxd boxes
Created attachment 352029 [details] [review] [4/4] smoothstreaming: Use isoff to parsing tfxd/tfrf
Hello. Thanks for updating the set. Your current version seems OK at a first glance but it's making the isoff test fail due to a missing _BAD_ include path on check/Makefile.am. Additionally: why is gstisoff.c being included by check/elements/isoff.c? I don't think that's needed. Please double check.
Review of attachment 352026 [details] [review]: ::: tests/check/Makefile.am @@ +451,3 @@ elements_dash_mpd_SOURCES = elements/dash_mpd.c +elements_isoff_CFLAGS = $(AM_CFLAGS) $(GST_BASE_CFLAGS) -elements_isoff_CFLAGS = $(AM_CFLAGS) $(GST_BASE_CFLAGS) +elements_isoff_CFLAGS = $(AM_CFLAGS) $(GST_BASE_CFLAGS) $(GST_PLUGINS_BAD_CFLAGS) (See previous comment) ::: tests/check/elements/isoff.c @@ +1,2 @@ +#include "../../gst-libs/gst/isoff/gstisoff.c" +#undef GST_CAT_DEFAULT -#include "../../gst-libs/gst/isoff/gstisoff.c" -#undef GST_CAT_DEFAULT - DITTO. Do double check though.
(In reply to Reynaldo H. Verdejo Pinochet from comment #24) > Hello. Thanks for updating the set. > > Your current version seems OK at a first glance > but it's making the isoff test fail due to a missing > _BAD_ include path on check/Makefile.am. Additionally: > why is gstisoff.c being included by check/elements/isoff.c? > I don't think that's needed. Please double check. Thanks for review. Inclusion of "gstisoff.c" seems not to be needed
Created attachment 352066 [details] [review] [1/4] isoff: Move isoff to gst-libs
Review of attachment 352066 [details] [review]: Almost there, two nits though: ::: gst-libs/gst/isoff/gstisoff.c @@ +40,3 @@ + } + +/* gst_isoff_parse_box: Wrong name, not vital but please fix if you are uploading a new iteration ::: tests/check/Makefile.am @@ +261,3 @@ elements/h263parse \ elements/h264parse \ + elements/isoff \ Non element-specfic isoff tests should probably be in libs/ Please double check and adjust accordingly.
Created attachment 352559 [details] [review] isoff: Move isoff to gst-libs
Created attachment 352560 [details] [review] [2/4] isoff: Add parsing moov and tfdt
Created attachment 352561 [details] [review] [3/4] isoff: Add parsing mss specific tfrf and tfxd boxes
(In reply to Reynaldo H. Verdejo Pinochet from comment #28) > Review of attachment 352066 [details] [review] [review]: > > Almost there, two nits though: > > ::: gst-libs/gst/isoff/gstisoff.c > @@ +40,3 @@ > + } > + > +/* gst_isoff_parse_box: > > Wrong name, not vital but please fix if you are uploading a new iteration Sorry for late reply. I fixed it. > > ::: tests/check/Makefile.am > @@ +261,3 @@ > elements/h263parse \ > elements/h264parse \ > + elements/isoff \ > > Non element-specfic isoff tests should > probably be in libs/ Please double check > and adjust accordingly. I think moving it into libs seems correct, and did it on new patch set.
Review of attachment 352560 [details] [review]: Thanks for staying on top of this. Appreciated. Code can be improved a bit but I see no blockers. Just a few suggestions: ::: gst-libs/gst/isoff/gstisoff.c @@ +425,3 @@ + if (!gst_byte_reader_skip (reader, 8)) + return FALSE; + Looks like the skip is actually the only conditional step and you can take the _get_uint32_be() outside the if/else if/else. Also, a switch would make the version logic easier to maintain and seems just better suited for the task. @@ +482,3 @@ + } + case GST_ISOFF_FOURCC_HDLR:{ + Nit, feel free to ignore: move sub_reader declaration to the top and save writing it twice? @@ +496,3 @@ + } + + return FALSE; There's no need for two $had_ variables. Use only one and consider making the function return the found value instead of a boolean if you think it's useful. @@ +526,3 @@ + } else { + return FALSE; + guint64 size; If future new $version values are possible please make it a switch. If not the whole block may be replaced by something simpler like (double check, drafted): if (version != 0 && version != 1) return FALSE; if (!gst_byte_reader_skip (reader, version? 8:16)) return FALSE; @@ +562,3 @@ + } + case GST_ISOFF_FOURCC_TKHD:{ + case GST_ISOFF_FOURCC_HDLR:{ Nit, feel free to ignore: move sub_reader declaration to the top and save writing it twice? @@ +578,3 @@ + } + + } Again, there's no need for two $had_ with the logic in place. Make it one and consider returning an mdia or tkhd flag if worth anything. @@ +619,3 @@ + default: + gst_byte_reader_skip (reader, size - header_size); + return FALSE; At a first glance it seems like you can drop all the had_track variable logic if you simple "goto error;" before breaking.
Review of attachment 352561 [details] [review]: Nothing severe but worth taking a look ... ::: gst-libs/gst/isoff/gstisoff.c @@ +297,3 @@ + memset (tfxd, 0, sizeof (*tfxd)); + + guint64 absolute_time = 0; How much would it take to get rid of the fixme? @@ +323,3 @@ + duration = ~duration; + absolute_time = ~time; + GST_ERROR ("Error getting box's flags field"); assignment block can be replaced by byte_reader_get(...); byte_reader_get(...); absolute_time = time; absolute_duration = duration; and then the following two (while correct) also become unnecessary: time = ~time; duration = ~duration; or maybe I'm missing something? @@ +329,3 @@ + tfxd->duration = absolute_duration; + + The _LOG() call here is redundant considering the info provided can be derived from the return value (The function only returns TRUE if the box has been parsed) @@ +351,3 @@ + GST_ERROR ("Error getting box's flags field"); + goto error; + gst_byte_reader_get_uint32_be (reader, &duration); seems like factoring out the two above checks from this function and _tfxd_box_parse() wouldn't hurt. Make it an inline func if you do. @@ +383,3 @@ + duration = ~duration; + absolute_time = ~time; +{ Please take a look at the above comment for the similar code in gst_isoff_tfxd_box_parse() @@ +391,3 @@ + } + + guint8 index = 0; DITTO, redundant log message.
Review of attachment 352029 [details] [review]: Looks good to me. ::: ext/smoothstreaming/gstmssfragmentparser.c @@ +41,3 @@ { + if (parser->moof) + gst_isoff_moof_box_free (parser->moof); Can you think of a reason why not making the _box_free () functions NULL-safe?
Review of attachment 352559 [details] [review]: I intend to push the whole set once the latest comments have been addressed. Thanks for staying on top of this.
(In reply to Reynaldo H. Verdejo Pinochet from comment #33) > Review of attachment 352560 [details] [review] [review]: Always thanks for your detailed review :) I'd like to ask and answer about your suggestion * About version handling, I prefer to use if/else instead of switch because - ISO/IEC 14496-12:2015 is defining like if/else and qtdemux also did it. So I'd like to make them similar. In my opinion, defining more version is not probable in near future. * About $had_.. variables My intention was to check existence of target child boxes on that boxes. For example, in case of gst_isoff_mdia_box_parse() which is for parsing mdia box, both mdhd and hdlr are mandatory (by spec, and for my use case) child boxes. So I defined two $had_ variables to check them. If one of them are omitted, it's error cases. However, since mdia box might have other unknown boxes as its child box (it's not error), we should iterate all bytes to search them. In this context, could you please inform more detail about following comment? > There's no need for two $had_ variables. Use only one and consider making the > function return the found value instead of a boolean if you think it's useful. > ... > At a first glance it seems like you can drop all the had_track variable > logic if you simple "goto error;" before breaking.
(In reply to Reynaldo H. Verdejo Pinochet from comment #35) > Review of attachment 352029 [details] [review] [review]: > > Looks good to me. > > ::: ext/smoothstreaming/gstmssfragmentparser.c > @@ +41,3 @@ > { > + if (parser->moof) > + gst_isoff_moof_box_free (parser->moof); > > Can you think of a reason why not making the _box_free () functions > NULL-safe? Nothing reasonable purpose I think :) I just reused existing one.
Created attachment 353136 [details] [review] [2/4] isoff: Add parsing moov and tfdt
Created attachment 353137 [details] [review] [3/4] isoff: Add parsing mss specific tfrf and tfxd boxes
(In reply to Reynaldo H. Verdejo Pinochet from comment #34) > Review of attachment 352561 [details] [review] [review]: > ::: gst-libs/gst/isoff/gstisoff.c > @@ +297,3 @@ > + memset (tfxd, 0, sizeof (*tfxd)); > + > + guint64 absolute_time = 0; > > How much would it take to get rid of the fixme? Removed fixme and added code for checking exact required size. > @@ +323,3 @@ > + duration = ~duration; > + absolute_time = ~time; > + GST_ERROR ("Error getting box's flags field"); > Couldn't find any API for both getting and ~= operation at once from gstbytereader.c... > @@ +329,3 @@ > + tfxd->duration = absolute_duration; > + > + > > The _LOG() call here is redundant considering the info provided can be > derived from the return value (The function only returns TRUE if the box has > been parsed) Done > > @@ +351,3 @@ > + GST_ERROR ("Error getting box's flags field"); > + goto error; > + gst_byte_reader_get_uint32_be (reader, &duration); > > seems like factoring out the two above checks from this function and > _tfxd_box_parse() wouldn't hurt. Make it an inline func if you do. Should we get rid of them from this function?
Review of attachment 353136 [details] [review]: (In reply to Seungha Yang from comment #37) > > Always thanks for your detailed review :) > Thanks for keeping up with it! > I'd like to ask and answer about your suggestion > > * About version handling, > I prefer to use if/else instead of switch because > - ISO/IEC 14496-12:2015 is defining like if/else and qtdemux also did it. > So I'd like to make them similar. In my opinion, defining more version is > not probable in near future. Famous last words :D but It's no biggie. Leave it as it is. > [..] > In this context, could you please inform more detail about following comment? > > There's no need for two $had_ variables. Use only one and consider making the > function return the found value instead of a boolean if you think it's useful. > > ... I meant that your logic can be reproduced with only one variable. But it's OK, your version is actually clearer. On the function's return, I was suggesting using it to return information on what was actually missing. New version looks good to go.
(In reply to Seungha Yang from comment #41) > (In reply to Reynaldo H. Verdejo Pinochet from comment #34) > > Review of attachment 352561 [details] [review] [review] [review]: > > > ::: gst-libs/gst/isoff/gstisoff.c > > @@ +297,3 @@ > > + memset (tfxd, 0, sizeof (*tfxd)); > > + > > + guint64 absolute_time = 0; > > > > How much would it take to get rid of the fixme? > Removed fixme and added code for checking exact required size. > Thank you! > > > @@ +323,3 @@ > > + duration = ~duration; > > + absolute_time = ~time; > > + GST_ERROR ("Error getting box's flags field"); > > > > Couldn't find any API for both getting and ~= operation at once from > gstbytereader.c... Can you elaborate on why are you applying bitwise-NOT operations _twice_ on time & duration before assigning the resulting values to the absolute_ vars? > > > > @@ +329,3 @@ > > + tfxd->duration = absolute_duration; > > + > > + > > > > The _LOG() call here is redundant considering the info provided can be > > derived from the return value (The function only returns TRUE if the box has > > been parsed) > Done > > > > > @@ +351,3 @@ > > + GST_ERROR ("Error getting box's flags field"); > > + goto error; > > + gst_byte_reader_get_uint32_be (reader, &duration); > > > > seems like factoring out the two above checks from this function and > > _tfxd_box_parse() wouldn't hurt. Make it an inline func if you do. > > Should we get rid of them from this function? If you want. Thanks again for staying on top of this.
Created attachment 354783 [details] [review] [3/4] isoff: Add parsing mss specific tfrf and tfxd boxes
(In reply to Reynaldo H. Verdejo Pinochet from comment #43) > > > @@ +323,3 @@ > > > + duration = ~duration; > > > + absolute_time = ~time; > > > + GST_ERROR ("Error getting box's flags field"); > > > > > > > Couldn't find any API for both getting and ~= operation at once from > > gstbytereader.c... > > > Can you elaborate on why are you applying bitwise-NOT operations _twice_ on > time & duration before assigning the resulting values to the absolute_ vars? I removed them. It was copied from original code in "gstmssfragmentparser.c" Assigning uint32 value to uint64 must have no problem. The bitwise operation seems to be needless
Review of attachment 354783 [details] [review]: LGTM. Thank you!
Pushed as: 0f1de50222e7fb5b7... smoothstreaming: Use isoff to parse tfxd/tfrf 98576325e31fe3550... isoff: Add parsing mss specific tfrf and tfxd boxes 3db9152ec6a9a55b0... isoff: Add parsing moov and tfdt 7d06ecb3a4f1344a1... isoff: Move isoff to gst-libs
Review of attachment 352559 [details] [review]: commit 7d06ecb3a4f1344a1ea146c818c73a83ce137661 Author: Seungha Yang <sh.yang@lge.com> Date: Thu May 25 18:14:09 2017 +0900 isoff: Move isoff to gst-libs Also rename unit test dash_isoff to isoff https://bugzilla.gnome.org/show_bug.cgi?id=777825
Review of attachment 352029 [details] [review]: commit 0f1de50222e7fb5b738ad89dd060701add450699 (HEAD -> master, origin/master, origin/HEAD, isoff) Author: Seungha Yang <sh.yang@lge.com> Date: Wed May 17 22:09:48 2017 +0900 smoothstreaming: Use isoff to parse tfxd/tfrf https://bugzilla.gnome.org/show_bug.cgi?id=777825
Review of attachment 353136 [details] [review]: commit 3db9152ec6a9a55b009dcc9d5f02ec60080af4db Author: Seungha Yang <sh.yang@lge.com> Date: Fri Jun 2 23:19:36 2017 +0900 isoff: Add parsing moov and tfdt To extract isobmff level timestamp, moov and tfdt parsing is required.
Review of attachment 354783 [details] [review]: commit 98576325e31fe355022bb2d9bfaffd12f5c02c46 Author: Seungha Yang <sh.yang@lge.com> Date: Sun Jul 2 14:27:33 2017 +0900 isoff: Add parsing mss specific tfrf and tfxd boxes This code is imported from mssdemux's tfxd/tfrf parsing function