GNOME Bugzilla – Bug 748507
mpegtsmux: set non-0 payload length in PES header if video ES packet is small enough
Last modified: 2016-01-19 19:36:14 UTC
Currently, in case of video ES, it always set 0 to the length of pes. According to ISO spec, this is right, and working fine. But doesn't it need to set length when the length is lesser than 16bit max (in case of video ES)? Is there any specific reason to set 0 in all video ES? How about this logic? if [ length is bigger than 16bit max ] set to 0 else set to length I'm using mpegtsmux in my project, and I'm thinking about this change. Could you give me some advices for this?
Created attachment 302549 [details] [review] mpegtsmux: set non-0 payload length in PES header if video ES packet is small enough Change to set non-0 video payload size if video ES packet is small enough. Fix a bug found that pes_bytes_written is not to reset if unbounded stream.
Comment on attachment 302549 [details] [review] mpegtsmux: set non-0 payload length in PES header if video ES packet is small enough >Fix a bug found that pes_bytes_written is not to reset if unbounded stream. Perhaps this bug fix should be done in a separate patch? > /* FIXME: As a hack, for unbounded streams, start a new PES packet for each > * incoming packet we receive. This assumes that incoming data is >- * packetised sensibly - ie, every video frame */ >- if (stream->cur_pes_payload_size == 0) >+ * packetised sensibly - ie, some video frames, which have length over 16 bit */ I don't think this comment needs to be changed. What the comment says is that the assumption is that the incoming stream is parsed. I don't see how your size patch changes any of that or is affected by that. > } else if (stream->is_video_stream) { >- /* Unbounded for video streams */ >- stream->cur_pes_payload_size = 0; >- tsmux_stream_find_pts_dts_within (stream, stream->bytes_avail, &stream->pts, >- &stream->dts); >+ if (stream->bytes_avail > G_MAXUINT16) { >+ /* Unbounded for video streams if size is over 16 bit */ >+ stream->cur_pes_payload_size = 0; no tsmux_stream_find_pts_dts_within() needed here? >+ } else { >+ stream->cur_pes_payload_size = stream->bytes_avail; >+ tsmux_stream_find_pts_dts_within (stream, stream->cur_pes_payload_size, >+ &stream->pts, &stream->dts); >+ }
> >Fix a bug found that pes_bytes_written is not to reset if unbounded stream. > > Perhaps this bug fix should be done in a separate patch? Yes. But it's not producible on current logic. Because pes_bytes_written is reset in case of bounded stream, while it is not needed to be reset in case of unbounded stream for all video packets. But I think this fix is needed. > I don't think this comment needs to be changed. What the comment says is > that the assumption is that the incoming stream is parsed. I don't see how > your size patch changes any of that or is affected by that. You're right :-) > no tsmux_stream_find_pts_dts_within() needed here? Oops. I didn't catch it. Thanks for pointing out.
Created attachment 304764 [details] [review] reset pes_bytes_written when starting to write new PES packet Split the patch into two patches. This one is bug-fix.
Created attachment 304765 [details] [review] set non-0 payload length in PES header if video ES packet is small enough Another is to set non-0 payload length patch with Tim's review.
Another question: doesn't the PES packet length include some extended header bytes as well? If yes, wouldn't you want to check for something smaller than G_MAXUINT16 so you can add the header bytes without overflowing?
(In reply to Tim-Philipp Müller from comment #6) > Another question: doesn't the PES packet length include some extended header > bytes as well? If yes, wouldn't you want to check for something smaller than > G_MAXUINT16 so you can add the header bytes without overflowing? Oh.. That's right. I didn't think of it. It should consider header length, which is variable. I think I have to provide new patch after thinking.
Created attachment 307387 [details] [review] mpegtsmux: set non-0 payload length in PES header if video ES packet is small enough Move comparison operation to where it can get valid pes header length. IMO, it doesn't affect getting dts and pts.
Created attachment 307388 [details] [review] mpegtsmux: reset pes_bytes_written when starting to write new PES packet Rebased on the latest git master
Thanks! Sorry it took so long to get these in! commit a274299f48799c12f5cf0e7a62ba52edcc49bbf9 Author: Hyunjun Ko <zzoon.ko@samsung.com> Date: Tue Jul 14 13:40:46 2015 +0900 mpegtsmux: set non-0 payload length in PES header if video ES packet is small enough https://bugzilla.gnome.org/show_bug.cgi?id=748507 commit f9ef150652012d602fb293c999e5b2072b2da8f3 Author: Hyunjun Ko <zzoon.ko@samsung.com> Date: Tue Jul 14 13:42:54 2015 +0900 mpegtsmux: reset pes_bytes_written when starting to write new PES packet In case of an unbounded packet (video usually), pes_bytes_written was no reset. https://bugzilla.gnome.org/show_bug.cgi?id=748507