GNOME Bugzilla – Bug 749328
hlsdemux: Simplify logic in process_manifest
Last modified: 2015-06-05 12:50:17 UTC
I have modified logic in process_manifest to fix a TODO item.
Created attachment 303327 [details] [review] hlsdemux: Simplify logic in process_manifest
commit 4ca3a22b6b33ad8be4383063e76f79c4d346535d Author: Jimmy Ohn <yongjin.ohn@lge.com> Date: Thu May 14 02:11:50 2015 +0900 hlsdemux: Simplify logic in process_manifest Simplify logic in process_manifest and remove a TODO item. https://bugzilla.gnome.org/show_bug.cgi?id=749328
(In reply to Sebastian Dröge (slomo) from comment #2) > commit 4ca3a22b6b33ad8be4383063e76f79c4d346535d > Author: Jimmy Ohn <yongjin.ohn@lge.com> > Date: Thu May 14 02:11:50 2015 +0900 > > hlsdemux: Simplify logic in process_manifest > > Simplify logic in process_manifest and remove a TODO item. > > https://bugzilla.gnome.org/show_bug.cgi?id=749328 Thank you for your review :)
This has broken unit tests for hlsdemux. Some unit tests rely on trying ti get the variant with bitrate=0 to get the lowest one. From the function's name 'gst_m3u8_client_get_playlist_for_bitrate', having 0 meaning that it should return the current variant doesn't seem very clear. I'd recommend reverting the patch and trying a different alternative. As that code is not repeated in other parts, maybe it would make sense just to remove the original FIXME comment.
(In reply to Thiago Sousa Santos from comment #4) > This has broken unit tests for hlsdemux. Some unit tests rely on trying ti > get the variant with bitrate=0 to get the lowest one. > > From the function's name 'gst_m3u8_client_get_playlist_for_bitrate', having > 0 meaning that it should return the current variant doesn't seem very clear. > I'd recommend reverting the patch and trying a different alternative. As > that code is not repeated in other parts, maybe it would make sense just to > remove the original FIXME comment. Thanks for your comment. I agree your opinion. Also, I'm sorry about that I didn't test that code.:( Actually I didn't know where is hls test code. So, I heard from slomo where is that test code. I'll try a different alternative. Actually I just test below command when I test the hlsdemux. gst-launch souphttpsrc location=http://devimages.apple.com/iphone/samples/bipbop/gear4/prog_index.m3u8 ! hlsdemux ! decodebin2 ! videoconvert ! videoscale ! autovideosink
Created attachment 304304 [details] [review] hlsdemux: Simplify logic in process_manifest Accoding to the upper comment in hlsdemux code, We should select the first one in the variant playlist. Also, connection_speed 0 meaning that is download mode. So, We should select lowest one in the variant playlist I think.
When I check this unit test for hlsdemux, it's working for me. But I found another error like below. This is not related to my patch. elements/hlsdemux_m3u8.c:561:F:m3u8client:test_playlist_with_doubles_duration:0: 'file->duration' (10320999999) is not equal to '10.321 * (1000000 * (__extension__ (1000LL)))' (10321000000)
The tests were failing as it would try to get the minimum bitrate variant but would get the previous/default one with the new behavior. It seems to me that just dropping the TODO is enough on this case, after the connection-speed logic has been moved to the baseclass there isn't much left to fix here. commit 0a63fa7a01e82b00251f1a995d8cf14593d0fbfb Author: Thiago Santos <thiagoss@osg.samsung.com> Date: Fri Jun 5 09:22:58 2015 -0300 hlsdemux: drop TODO that doesn't need a solution Connection speed is only checked at that point in hlsdemux so there is no real need to refactor it. https://bugzilla.gnome.org/show_bug.cgi?id=749328 commit 581d8c0b8d0c3d7d4261282c8b1555e43deecb6b Author: Thiago Santos <thiagoss@osg.samsung.com> Date: Fri Jun 5 09:15:34 2015 -0300 Revert "hlsdemux: Simplify logic in process_manifest" This reverts commit 4ca3a22b6b33ad8be4383063e76f79c4d346535d. The connection-speed=0 is used as a special value in the property of hlsdemux to mean 'automatic' selection, m3u8.c doesn't need to know about that as it should be as simple as possible. So this patch hides this automatic selection documented in hlsdemux into m3u8 logic and I think the gets harder to understand the code. It also makes the hlsdemux unit tests work again https://bugzilla.gnome.org/show_bug.cgi?id=749328