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

Large pixellation for some MPEG2 clips (using Android SW MPEG2 decoder) #2891

Closed
goffioul opened this issue Jun 1, 2017 · 21 comments
Closed
Assignees
Labels

Comments

@goffioul
Copy link
Contributor

goffioul commented Jun 1, 2017

I'm using a Intel-based device running Android-IA (Nougat), where I've enabled the MPEG2 codec in media_codecs.xml (SW-decoder available in Android since Marshmallow). My app is using ExoPlayer r2.4.1 (with ffmpeg extension built-in). I'm testing various MPEG clips using plain HTTP. Most of the MPEG2 clips play fine, but a few of them show massive pixellation. One sample is available at https://drive.google.com/open?id=0BwljeX6541LuamdieXZzamNkNFE . Those clips play fine in any other player.

As a comparison, here's a screenshot of how it looks like on Android:
https://drive.google.com/open?id=0BwljeX6541LuSW1BYW9YamlLeGM
Here's what the same looks like in VLC on Linux:
https://drive.google.com/open?id=0BwljeX6541LuM1B5ZnoycDNndms

Is the stream not compatible with ExoPlayer? Or with Android MPEG2 decoder? Is Android MPEG2 decoder simply doing a very poor job?

@ojw28
Copy link
Contributor

ojw28 commented Jun 1, 2017

Sounds like you're manually enabling a decoder yourself in the platform, and observing that it doesn't work very well. Isn't that most likely to be an issue with the decoder? What decoder is it that's actually being used?

There's nothing we can actually do to look at this issue given the lack of a bug report, device details and so on (edit: ignore previous comment about test content).

@goffioul
Copy link
Contributor Author

goffioul commented Jun 1, 2017

I've sent the result of "adb bugreport' by email. The device is an Intel/Baytrail platform running Android-IA.

The decoder is the one from Google, available in AOSP since marshmallow [1]. It used to be enabled by default [2], but for some reason this was changed on nougat [3]. The decoder is built into the firmware anyway, so I simply enabled it by including media_codecs_google_tv.xml into main media_codecs.xml.

[1] https://android.googlesource.com/platform/frameworks/av/+/android-6.0.0_r1/media/libstagefright/codecs/mpeg2dec/
[2] https://android.googlesource.com/platform/frameworks/av/+/android-6.0.0_r1/media/libstagefright/data/media_codecs_google_video.xml
[3] https://android.googlesource.com/platform/frameworks/av/+/e1c9766e23a1e5084d9fdbcc0b2005406627f1d4

@ojw28
Copy link
Contributor

ojw28 commented Jun 1, 2017

If it's a Google codec then you should probably file an issue on Android directly, as described here, under GfxMedia presumably.

@goffioul
Copy link
Contributor Author

goffioul commented Jun 1, 2017

I was hoping you could inspect the sample clip to determine what special about it that can trigger pixellation, while the same decoder works fine with other clips.

@ojw28
Copy link
Contributor

ojw28 commented Jun 2, 2017

This is beyond what we realistically have time for, particularly given the decoder isn't enabled by default. Sorry. Please do file an issue on Android directly if you want someone to take a look. Thanks.

@ojw28 ojw28 closed this as completed Jun 2, 2017
@goffioul
Copy link
Contributor Author

goffioul commented Jun 6, 2017

I've made some experiments with ffmpeg, emulating the behavior of H262Reader. The strange thing was that I also got pixellation when using my experiment code, while ffplay could play the stream just fine. It appears that in the case of ffmpeg, the pixellation is due to the way the data is segmented and fed to the decoder.

Based on H262Reader, I've segmented my data on picture start code boundaries (0x00 0x00 0x01 0x00). But it appears that when doing that, the decoder ignores any sequence and GOP headers that are embedded in the data chunks. My sample stream has additional sequence and GOP headers in the middle of the stream. These contains custom decoding parameters (quantisation matrix). If these are ignored by the decoder, decoding artefacts then appear.

To make it work with ffmpeg, one must segment the data properly before feeding it to the decoder. In particular, the sequence and GOP headers must be isolated and fed separately.

Given that similar pixellation problems appear when using the MPEG2 software codec from Google, I'd say it's possible it has the same cause, aka the way H262Reader segments the data.

@ojw28
Copy link
Contributor

ojw28 commented Jun 7, 2017

Does this imply you know how to fix it too ;)? Feel free to send a pull request if so. It's quite low priority for us, so it'll almost certainly be much faster for you to fix it.

@goffioul
Copy link
Contributor Author

goffioul commented Jun 7, 2017

This (temporary/experimental) patch fixes the problem. The idea is to segment data at picture, sequence and GOP header boundaries. However, I ran into an exception while playing another sample clip that contains a discontinuity.

The patch is available here:
https://drive.google.com/open?id=0BwljeX6541Luak1oMldUS3FVZjQ
The clip is available here:
https://drive.google.com/open?id=0BwljeX6541LualNuV0VTSXFWSlk

The exception is:

Internal runtime error.
java.util.NoSuchElementException
	at java.util.concurrent.LinkedBlockingDeque.removeFirst(LinkedBlockingDeque.java:424)
	at java.util.concurrent.LinkedBlockingDeque.remove(LinkedBlockingDeque.java:643)
	at com.google.android.exoplayer2.extractor.DefaultTrackOutput.dropDownstreamTo(DefaultTrackOutput.java:429)
	at com.google.android.exoplayer2.extractor.DefaultTrackOutput.readData(DefaultTrackOutput.java:295)
	at com.google.android.exoplayer2.source.ExtractorMediaPeriod.readData(ExtractorMediaPeriod.java:339)
	at com.google.android.exoplayer2.source.ExtractorMediaPeriod$SampleStreamImpl.readData(ExtractorMediaPeriod.java:573)
	at com.google.android.exoplayer2.BaseRenderer.readSource(BaseRenderer.java:274)
	at com.google.android.exoplayer2.mediacodec.MediaCodecRenderer.feedInputBuffer(MediaCodecRenderer.java:621)
	at com.google.android.exoplayer2.mediacodec.MediaCodecRenderer.render(MediaCodecRenderer.java:511)
	at com.google.android.exoplayer2.ExoPlayerImplInternal.doSomeWork(ExoPlayerImplInternal.java:479)
	at com.google.android.exoplayer2.ExoPlayerImplInternal.handleMessage(ExoPlayerImplInternal.java:308)
	at android.os.Handler.dispatchMessage(Handler.java:98)
	at android.os.Looper.loop(Looper.java:154)
	at android.os.HandlerThread.run(HandlerThread.java:61)

@goffioul
Copy link
Contributor Author

I've noticed something unexpected regarding pixellation. If I change the container to mkv, keeping all elementary streams unchanged, then pixellation does not occur. I've changed container with:

ffmpeg -i SOURCE.ts -codec copy -map 0 SOURCE.mkv

I've extracted the elementary video stream from both TS and MKV files and they're identical at the byte level. So I guess somehow data is segmented differently when using MKV.

Any suggestion about where to look?

Note: tested with this sample
https://drive.google.com/open?id=0BwljeX6541LubXprdmEtTFdBT00

@goffioul
Copy link
Contributor Author

goffioul commented Jul 4, 2017

I've updated the patch for the branch dev-v2, you can find it here:
https://drive.google.com/open?id=0BwljeX6541LuSTg5ZzBTcHBCNEU

I'm still getting an exception thrown when playing the test clip with the discontinuity:

AudioTrack: Discontinuity detected [expected 89621722, got 89909722]
ExoPlayerImplInternal: Internal runtime error.
ExoPlayerImplInternal: java.lang.NullPointerException: Attempt to read from field 'long com.google.android.exoplayer2.source.SampleQueue$AllocationNode.endPosition' on a null object reference
ExoPlayerImplInternal: 	at com.google.android.exoplayer2.source.SampleQueue.advanceReadTo(SampleQueue.java:487)
ExoPlayerImplInternal: 	at com.google.android.exoplayer2.source.SampleQueue.readData(SampleQueue.java:444)
ExoPlayerImplInternal: 	at com.google.android.exoplayer2.source.SampleQueue.read(SampleQueue.java:351)
ExoPlayerImplInternal: 	at com.google.android.exoplayer2.source.ExtractorMediaPeriod.readData(ExtractorMediaPeriod.java:353)
ExoPlayerImplInternal: 	at com.google.android.exoplayer2.source.ExtractorMediaPeriod$SampleStreamImpl.readData(ExtractorMediaPeriod.java:592)
ExoPlayerImplInternal: 	at com.google.android.exoplayer2.BaseRenderer.readSource(BaseRenderer.java:277)
ExoPlayerImplInternal: 	at com.google.android.exoplayer2.mediacodec.MediaCodecRenderer.feedInputBuffer(MediaCodecRenderer.java:631)
ExoPlayerImplInternal: 	at com.google.android.exoplayer2.mediacodec.MediaCodecRenderer.render(MediaCodecRenderer.java:521)
ExoPlayerImplInternal: 	at com.google.android.exoplayer2.ExoPlayerImplInternal.doSomeWork(ExoPlayerImplInternal.java:557)
ExoPlayerImplInternal: 	at com.google.android.exoplayer2.ExoPlayerImplInternal.handleMessage(ExoPlayerImplInternal.java:333)
ExoPlayerImplInternal: 	at android.os.Handler.dispatchMessage(Handler.java:98)
ExoPlayerImplInternal: 	at android.os.Looper.loop(Looper.java:154)
ExoPlayerImplInternal: 	at android.os.HandlerThread.run(HandlerThread.java:61)

@ojw28
Copy link
Contributor

ojw28 commented Jul 4, 2017

That failure implies your patch is writing incorrect data to the track output. For example, the values passed to output.sampleMetadata may imply the data of one sample overlaps that of another, or may refer to data that hasn't been written to the output yet.

The intended segmentation of the stream into samples is that each sample should produce a corresponding output buffer after decoding. Is your patch a step toward achieving that, or is that what the implementation already does? And if that's what the implementation already does, is your patch segmenting such that there are more samples than we expect corresponding output buffers after decoding?

@goffioul
Copy link
Contributor Author

goffioul commented Jul 5, 2017

The patch is intended to fix the segmentation of ExoPlayer, which is not suited to decoders from ffmpeg or Google. By not segmenting data into logical entities, the decoders miss important decoding parameters leading to pixellation and artefacts.

@ojw28
Copy link
Contributor

ojw28 commented Jul 5, 2017

That doesn't answer the question. The "correct" segmentation is that each input buffer to the decoder should correspond to a single output frame. The question I'm asking is whether your change achieves this, or whether this is already the case in the current version of your code, and your change is moving away from this. Which one of these is true?

If the segmentation is already correct and the decoders are missing things, then we should get the decoders fixed and probably implement a re-segmentation workaround that's closer to where the decoder is used in the code-base, rather than being all the way up in an extractor that has no idea what decoder the samples are eventually going to be fed into.

@goffioul
Copy link
Contributor Author

goffioul commented Jul 5, 2017

My patch was a deviation from the rule "1 sample -> 1 output buffer", which lead to the exception in case of discontinuities. Nevertheless, the segmentation in H262Reader is incorrect and I finally found the correct way that prevents the pixellation problem and does not break the above rule. Thanks.

@ojw28
Copy link
Contributor

ojw28 commented Jul 5, 2017

Ah, that's good news! Please let us know if/when you have an updated patch.

@ojw28
Copy link
Contributor

ojw28 commented Jul 16, 2017

@goffioul - Could you provide a pull request or explanation describing the correct segmentation? Thanks.

@andrewlewis
Copy link
Collaborator

@goffioul Thanks for the pull request. We'll review this and get it merged soon.

As an aside, do you happen to know whether there is a guarantee that sequence header start codes always occur at the exact start of a packet, or whether it's ever possible for the start codes to straddle a packet boundary? I think our current implementation doesn't handle this case correctly, but I wanted to check if it can occur before submitting a fix. I'm not familiar with this format but it seems strange that CsdBuffer.onStartCode handles the next start code being across a packet boundary but not the sequence header or extension header start code. Thanks!

@goffioul
Copy link
Contributor Author

@andrewlewis I'm not sure what you mean with "packet". Are you referring to the MPEG-TS packets? If yes, I think any start code could straddle across packet boundaries, and that's not the point of the proposed patch. The H262Reader class handles straddling start codes fine, I believe.

The problem is how H262Reader segments the mpeg2video elementary stream into data chunk to be sent to the decoder. Especially when a frame sequence ends and a new one starts, the data stream would look like this:

... [FRAME_N] [SEQ] [GOP] [FRAME_0] ...

With FRAME_N being the last frame of the previous sequence, and FRAME_0 being the first frame of the next sequence (it's an I-frame). The part SEQ may contain new quantization factors that are applicable starting from FRAME_0. The existing H262Reader would segment the above data into 2 chunks/buffers, sent to the decoder:

chunk: [FRAME_N] [SEQ] [GOP]
chunk: [FRAME_0]

In the 2 decoders I've tested with (Android/Google's MPEG2 codec, and FFMPEG), the parts [SEQ] and [GOP] are ignored when they are appended like that to a frame. Those decoders expect [SEQ] and [GOP] to be prepended to the (first) frame they apply to instead. This then leads to incorrect decodings, as the quantization matrix from [SEQ] is not taken into account. The proposed patch segments the data into the following buffers instead:

chunk: [FRAME_N]
chunk: [SEQ] [GOP] [FRAME_0]

@andrewlewis
Copy link
Collaborator

@goffioul Yes, I mean MPEG-TS packets. The issue is with how the CsdBuffer fills. If the start code for a sequence header begins either at the final byte of a packet or the one before, isFilling is set in onStartCode after the first byte or two of the relevant header has been passed to onData already, which means that the bitstream parsed in parseCsdBuffer can be offset by one or two bytes. (And sequenceExtensionPosition is also not offset by the number of bytes already passed.)

I appreciate that your patch isn't trying to address this but noticed the problem in the existing code while reviewing it, and wanted to check if you knew whether there were guarantees about the positions of start codes. It sounds like there's no guarantee this situation can't arise, so I will make a fix. If you have test streams with lots of sequence headers it would be interesting to know if the sequence headers ever occur with their start codes at the end of a packet.

@goffioul
Copy link
Contributor Author

goffioul commented Aug 1, 2017

@andrewlewis Got it, thanks. It's indeed a separate issue. I don't know whether there's a guarantee to have the sequence headers at the start of a packet. I have several sample clips that you may find useful:
https://drive.google.com/open?id=0BwljeX6541LubXprdmEtTFdBT00
https://drive.google.com/open?id=0BwljeX6541LuQVM3d0NPcUduSWs
https://drive.google.com/open?id=0BwljeX6541LualNuV0VTSXFWSlk
https://drive.google.com/open?id=0BwljeX6541LucVliTUVaSmoyZk0
https://drive.google.com/open?id=0BwljeX6541LuamdieXZzamNkNFE
https://drive.google.com/open?id=0BwljeX6541LuVHQwY09pOU56SGM

On a related note, I submitted another pull request (#3114) related to CEA-608 support in mpeg2/mpegts. The code is largely based on CsdBuffer, so it's possible the problem you're referring to also affects the pull request.

ojw28 added a commit that referenced this issue Aug 2, 2017
Issue: #2891

-------------
Created by MOE: https:/google/moe
MOE_MIGRATED_REVID=163951910
@ojw28
Copy link
Contributor

ojw28 commented Aug 2, 2017

I made some further fixes to this. Please check everything is still working as you expect. Thanks!

@ojw28 ojw28 closed this as completed Aug 2, 2017
ojw28 added a commit that referenced this issue Aug 2, 2017
Issue: #2891

-------------
Created by MOE: https:/google/moe
MOE_MIGRATED_REVID=163951910
@google google locked and limited conversation to collaborators Dec 1, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

3 participants