Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

openmax h264 sps timing_info (fps) is not not generated #245

Closed
xlazom00 opened this issue Jan 7, 2014 · 19 comments
Closed

openmax h264 sps timing_info (fps) is not not generated #245

xlazom00 opened this issue Jan 7, 2014 · 19 comments

Comments

@xlazom00
Copy link

xlazom00 commented Jan 7, 2014

h264 SPS don't have timing_info fps information.
It looks like openmax api api don't propagate this value
from OMX_VIDEO_PORTDEFINITIONTYPE or OMX_VIDEO_PARAM_PORTFORMATTYPE
xFramerate for exampe.

@Ruffio
Copy link

Ruffio commented Jun 24, 2015

@xlazom00 is this still an issue?

@xlazom00
Copy link
Author

still
SPS has timing information only if we enable OMX_IndexParamBrcmVideoAVCSEIEnable
but that generate also H264 SEI

https://bitbucket.org/xlazom00/gst/src/096fda1174ee27d00d75df43543f69827faeefdd/patch/gst-omx/debian/patches/0001-aspect-ratio-information-SEI-NAL-unit.patch?at=master

@6by9
Copy link

6by9 commented Jun 24, 2015

I can see the code that controls this, but don't know enough about the bitstream to be happy to change it arbitrarily as it will change the behaviour for all clients of the codec.
It could be hidden behind an alternate param, but does that actually gain you anything?

@popcornmix what's your view? (h264_symgen.c!1148 wants to be conditional on something else, not SEI control flags) I guess if ffmpeg does look at this field, then it would mean all camera recordings would get the correct framerate when people throw the stream into a container, in which case perhaps it is a more sensible default.

@popcornmix
Copy link
Contributor

@6by9 is your thought just to drop the check for FLAG_ENABLE_SEI_TIMING? i.e.

if (enc->params.bitrate)

so we always write timing information?

@6by9
Copy link

6by9 commented Jun 26, 2015

I was thinking of a new flag, but probably setting video_encode's default to enable the timing with the option to turn it off.
We could go for just if (enc->params.bitrate), but I actually think that is a typo and should be enc->params.framerate. You may then have the option of just not telling video_encode what the framerate is to disable the timing info in the SPS.

I don't have enough of a feel for what impact this has for other use cases as it is too far down into the bitstream detail for my knowledge. Is Deborah still contracting for Pi Towers and able to comment? I've lost GV as a person to ask annoying questions.

@popcornmix
Copy link
Contributor

Yes, enc->params.bitrate seemed a surprising condition. No contact with @deborah-c for several weeks. @luke99 any opinion here?

@deborah-c
Copy link

Sorry: have been watching quietly, but I still don't really have the brainpower to do much useful with code.

I suspect that the test is like that because the source of the extra data required is the same, but it would be best to ask Graham

On 27 Jun 2015, at 17:41, popcornmix notifications@github.com wrote:

Yes, enc->params.bitrate seemed a surprising condition. No contact with @deborah-c for several weeks. @luke99 any opinion here?


Reply to this email directly or view it on GitHub.

@popcornmix
Copy link
Contributor

Hi @deborah-c. I guess the question is if enabling this by default will cause problems for any use cases. It seems unlikely to me (apart from a negligible increase in number of bits to encode). One option is to enable it and see if any problems emerge.

@6by9
Copy link

6by9 commented Jun 27, 2015

If I still shared an office with GV then I would have hassled him, but I haven't since about January :-( Seems a bit cheeky to ask these questions on Facebook.

@popcornmix I agree that it will only increase the size of the SPS by a few bits, and typically that isn't repeated.
I'll start making up a patch to plumb this in based on just enc->params.framerate. That is actually the framerate value set on the output port of the encoder, so I suspect it may well be set to 0 by default anyway....
ah no, this was a minor glitch I saw before. DEFAULT_FRAMERATE is set as 30fps. The value gets copied from input to output if output is set to 0fps. Almost never happens, and will therefore be 30. I think it caught jamesh out when writing raspivid as it threw the bitrate calcs out if they told the wrong framerate for the incoming data.
I probably ought to sort that at the same time and at least it will then copy the input port setting.

@6by9
Copy link

6by9 commented Jul 10, 2015

I have a patch that will hopefully work - I'll try to make up a test firmware over the weekend. A new parameter MMAL_PARAMETER_VIDEO_ENCODE_SPS_TIMING / OMX_IndexParamBrcmVideoAVCSPSTimingEnable to enable the extra metadata. It is disabled by default.

Actually the test on bitrate is valid as the SPS has bitrate related parameters in there which are independent of frame rate.

@6by9
Copy link

6by9 commented Oct 9, 2015

@xlazom00 Just noticed I still have my test commit sitting on my tree but I never sorted out the image to be tested. Is this still something you can test if I do sort out an image?

6by9 added a commit to 6by9/RPiTest that referenced this issue Nov 1, 2015
raspberrypi/firmware#440

Also adds an option to add SPS timing option into
H264 headers.
raspberrypi/firmware#245

Both features untested but believed correct.
@6by9
Copy link

6by9 commented Nov 1, 2015

Test firmware pushed to my RPiTest repo which includes an option that will hopefully set the requested parameter.
NB it takes the frame rate set on the OUTPUT port, but hopefully an extra amendment will copy it through from input to output port.

popcornmix added a commit that referenced this issue Nov 30, 2015
firmware: Camera: Set AWB mode on starting encode
See: raspberrypi/linux#1196

firmware: video_encode: option to add timing data to SPS header
See: #245

firmware: resize: add support for opaque images on the input
See: #440

firmware: Add IL ISP component

firmware: Add option to disable touchscreen on Pi display

firmware: hdmi: Accept EDID even if checksum is wrong on final read pt 2
See: http://openelec.tv/forum/124-raspberry-pi/74408-problems-with-hdmi-audio-on-openelec-5-0

firmware: dt-blob: Don't assign i2c0 to p28 and p29 on a Pi Zero
See: https://www.raspberrypi.org/forums/viewtopic.php?f=107&t=127292

firmware: platform: Bump default sdram to 450 on Pi0

firmware: mailbox: Add support for enabling/disabling v3d
popcornmix added a commit to Hexxeh/rpi-firmware that referenced this issue Nov 30, 2015
firmware: Camera: Set AWB mode on starting encode
See: raspberrypi/linux#1196

firmware: video_encode: option to add timing data to SPS header
See: raspberrypi/firmware#245

firmware: resize: add support for opaque images on the input
See: raspberrypi/firmware#440

firmware: Add IL ISP component

firmware: Add option to disable touchscreen on Pi display

firmware: hdmi: Accept EDID even if checksum is wrong on final read pt 2
See: http://openelec.tv/forum/124-raspberry-pi/74408-problems-with-hdmi-audio-on-openelec-5-0

firmware: dt-blob: Don't assign i2c0 to p28 and p29 on a Pi Zero
See: https://www.raspberrypi.org/forums/viewtopic.php?f=107&t=127292

firmware: platform: Bump default sdram to 450 on Pi0

firmware: mailbox: Add support for enabling/disabling v3d
popcornmix added a commit to raspberrypi/userland that referenced this issue Dec 8, 2015
@6by9
Copy link

6by9 commented Feb 1, 2016

@xlazom00 The change got merged. Any comment on it?

@xlazom00
Copy link
Author

it is ok now
you can close this issue

@xlazom00
Copy link
Author

btw
so we need to set OMX_IndexParamBrcmVideoAVCSPSTimingEnable to true to make it to work
It will be also nice if we enable this feature when we set framerate to encoder
So it will automagically just work as everybody expect :)

as I do with OMX_CONFIG_FRAMERATETYPE in my example
https://bitbucket.org/xlazom00/gst/src/096fda1174ee27d00d75df43543f69827faeefdd/patch/gst-omx/debian/patches/0001-aspect-ratio-information-SEI-NAL-unit.patch?at=master&fileviewer=file-view-default

@6by9
Copy link

6by9 commented Feb 13, 2016

It's off by default as I don't know enough about the decode side to know what most decoders expect.

@deborah-c do you have any knowledge of whether decoders really pay attention to this flag and is it actually worth setting by default?

@xlazom00
Copy link
Author

without fps information decoder don't know how to present video
if it is 25 or 30 or 60 fps
now decoder know only timestamps that is coded in muxer(mkv, mp4, .ts)
so it can match with 23,9999/25/30/60,.....
but :)

If you have for example camera that don't generate exact 25fps (CCTV cameras/system)
you don't need it as you just encode useful frames 20 frames here(person came to visible range) and 60frames here(mouse came) :)

but for movies/tv it is always constant so it is useful information

@6by9
Copy link

6by9 commented Feb 16, 2016

I've always encountered timestamps being stored in the container. I'm not aware of anything using the framerate from the H264 headers, hence why I wasn't wanting to enable this by default without good cause.

My knowledge is mainly from the camera and encode side, hence asking others for their opinions for general decoder behaviour.
There is the niggle with raspivid writing an elementary stream, which avconv assumes is 25fps by default if you try muxing it. If avconv picks up the framerate correctly from the headers then I would be happier to make it the default as it is of use to many, but would need to check client behaviour carefully (I had to make a subtle change to copying framerate from input to output port if not set).

@Ruffio
Copy link

Ruffio commented Jul 30, 2016

@xlazom00 if this issue has been fixed, please close it. Thanks

neuschaefer pushed a commit to neuschaefer/raspi-binary-firmware that referenced this issue Feb 27, 2017
firmware: Camera: Set AWB mode on starting encode
See: raspberrypi/linux#1196

firmware: video_encode: option to add timing data to SPS header
See: raspberrypi#245

firmware: resize: add support for opaque images on the input
See: raspberrypi#440

firmware: Add IL ISP component

firmware: Add option to disable touchscreen on Pi display

firmware: hdmi: Accept EDID even if checksum is wrong on final read pt 2
See: http://openelec.tv/forum/124-raspberry-pi/74408-problems-with-hdmi-audio-on-openelec-5-0

firmware: dt-blob: Don't assign i2c0 to p28 and p29 on a Pi Zero
See: https://www.raspberrypi.org/forums/viewtopic.php?f=107&t=127292

firmware: platform: Bump default sdram to 450 on Pi0

firmware: mailbox: Add support for enabling/disabling v3d
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants