GNOME Bugzilla – Bug 766803
h264parse: add support for config-interval=-1 to re-send SPS/PPS on I/IDR frames
Last modified: 2016-06-13 09:13:39 UTC
Created attachment 328393 [details] [review] Changed H.264 property config_interval to signed int To be able to use a negative value (-1) to In the represent sending SPS & PPS inband for each IDR frame we have channged the property "config-interval" from a unsigned int to an signed int.
Let's retitle this to what you actually want to achieve, you can then attach the other patch here too :)
Sorry for being a bit densed. Should I change the commit-message? What do you want me to change it to, the same as the title of the bug?
My understanding was that you wanted to add support for config-interval=-1 to h264parse, in the same way as rtph264pay. This requires more than just changing the property from uint to int though, so I assumed there would be a follow-up patch that implements the support for config-interval=-1. That is the actual change, the uint->int change is just preparation as far as I can tell, no?
Sorry, now I see what you mean. I was not aware that the config-interval=-1 solution in rtph264pay was present in the upstream code. I thought that solution was Axis specific. My bad! I will supply you with the rest of the h264parse equivalent config-interval=-1 code ASAP tomorrow. I'm a bit new to this. :-)
Created attachment 328417 [details] [review] Add support for config-interval=-1 to re-send SPS/PPS on I/IDR frames The full config-interval=-1 solution for h264parse.
Comment on attachment 328417 [details] [review] Add support for config-interval=-1 to re-send SPS/PPS on I/IDR frames Thanks for the patch! >+ } else if (h264parse->interval == -1) { >+ if (h264parse->idr_pos >= 0) { >+ GST_LOG_OBJECT (h264parse, "IDR nal at offset %d", h264parse->idr_pos); >+ >+ gst_h264_parse_handle_sps_pps_nals (h264parse, buffer, frame); >+ >+ /* we pushed whatever we had */ >+ h264parse->push_codec = FALSE; >+ h264parse->have_sps = FALSE; >+ h264parse->have_pps = FALSE; >+ h264parse->state &= GST_H264_PARSE_STATE_VALID_PICTURE_HEADERS; >+ } > } Any reason this can't go as a simple || addition into the existing if/else branches? (Honest question, just curious to know if you've considered it and there's a reason or not).
Hi, I did consider putting the (h264parse->interval == -1) option int the existing if/else. I guess I did that consideration before I added the gst_h264_parse_handle_sps_pps_nals help function, when there were a lot more code to take into account. Anyhow I ended up with this solution since I found it a bit cleaner. It doesn't bother with time-intervals at all. There are some duplication of code but I didn't think it was too much. I guess I could check for (h264parse->interval == -1) twice and make the existing if/else work. Should I change it?
No it's fine, just trying to understand. Could you provide three separate patches please in this case: - one for the property type change (as attached already) - one for factoring out the sps/pps handling - one for adding the interval == -1 part
Created attachment 328491 [details] [review] Support for signed config-interval property
Created attachment 328493 [details] [review] Support for signed config-interval property
Created attachment 328502 [details] [review] Refactored handling of SPS/PPS when pushing frames.
Created attachment 328503 [details] [review] Support for handling of config-interval = -1.
Hi, I hope this is ok. I removed the old patches and added the fix in three separate patches.
Comment on attachment 328493 [details] [review] Support for signed config-interval property This one looks fine, but please adjust the commit message: > Support for signed config-interval property Should be something like: > h264parse: change "config-interval" property type from uint to int > > So we can use -1 as special value like in rtph264pay. > > https://bugzilla.gnome.org/show_bug.cgi?id=766803
Comment on attachment 328503 [details] [review] Support for handling of config-interval = -1. This one is fine, but please fix up the commit message (summary line, bug url).
Created attachment 329457 [details] [review] h264parse: change "config-interval" property type from uint to int
Created attachment 329458 [details] [review] h264parse: refactored handling of SPS/PPS when pushing frames
Created attachment 329459 [details] [review] h264parse: support for handling of config-interval = -1
Comment on attachment 329458 [details] [review] h264parse: refactored handling of SPS/PPS when pushing frames Here you have moved the h264parse->last_report = new_ts; from the inner loop during the refactoring, and are now doing it unconditionally. Someone needs to investigate if it matters or not. I think it's a bit suspicious, I think we might be resetting the timer/counter even if we didn't send anything yet. Perhaps it'd be better to make the new function return a boolean if it sent out stuff, and only reset the last_report time then?
Comment on attachment 329457 [details] [review] h264parse: change "config-interval" property type from uint to int >Subject: [PATCH 1/3] h264rse: change "config-interval" property type from uint Typo in summary, but we can fix this when we commit it.
(In reply to Tim-Philipp Müller from comment #19) > Comment on attachment 329458 [details] [review] [review] > h264parse: refactored handling of SPS/PPS when pushing frames > > Here you have moved the > > h264parse->last_report = new_ts; > > from the inner loop during the refactoring, and are now doing it > unconditionally. > > Someone needs to investigate if it matters or not. > > I think it's a bit suspicious, I think we might be resetting the > timer/counter even if we didn't send anything yet. > > Perhaps it'd be better to make the new function return a boolean if it sent > out stuff, and only reset the last_report time then? You could add new_ts as an function parameter and set it as it was done before? I think I had it like that in an earlier version of the code. Don't know why I changed my mind.
Created attachment 329662 [details] [review] h264parse: change "config-interval" property type from uint to int
Created attachment 329663 [details] [review] h264parse: refactored handling of SPS/PPS when pushing frames
Created attachment 329664 [details] [review] h264parse: support for handling of config-interval = -1
Pushed, thanks! commit a0876aa750797d9f171040d4736086fcfea55b91 Author: Mats Lindestam <matslm@axis.com> Date: Wed May 25 12:55:36 2016 +0200 h264parse: support for handling of config-interval = -1 Added support for handling of config-interval = -1. config-inteval = -1 represents resending SPS and PPS for each I-/IDR-frame. https://bugzilla.gnome.org/show_bug.cgi?id=766803 commit bf0d952387e517c983fce44bcf129db87e8b3ea3 Author: Mats Lindestam <matslm@axis.com> Date: Wed May 25 12:45:17 2016 +0200 h264parse: refactored handling of SPS/PPS when pushing frames https://bugzilla.gnome.org/show_bug.cgi?id=766803 commit 0c04e004bbd976c8c58b5b3528011e293bfaa34c Author: Mats Lindestam <matslm@axis.com> Date: Wed May 25 11:54:55 2016 +0200 h264parse: change "config-interval" property type from uint to int So we can use -1 as special value like in rtph264pay. https://bugzilla.gnome.org/show_bug.cgi?id=766803