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
[FIX] MP4 (from itunes) files not being processed (no subs) (#1223) #1233
Conversation
CCExtractor CI platform finished running the test files on windows. Below is a summary of the test results:
It seems that not all tests were passed completely. This is an indication that the output of some files is not as expected (but might be according to you). Your PR breaks these cases:
Check the result page for more info. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMHO going back to 0.86 is going to fix that regression but it's also going to remove some fixes that the original PR introduced, so I'd say it's a better idea to fix the actual bug that was introduced (which visually I can't see to be honest, so it will require some actual debugging).
|
||
if(process_avc_track(ctx, file, f, i + 1, &dec_sub) != 0) | ||
{ | ||
mprint("error\n"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I pretty much prefer errors message to be specific. Error on process_avc_track() is much better than "error" (same everywhere else).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will keep in mind.
switch (track_type) { | ||
case MEDIA_TYPE(GF_ISOM_MEDIA_VISUAL, GF_ISOM_SUBTYPE_XDVB): //vide:xdvb | ||
if ( type == GF_ISOM_MEDIA_VISUAL && subtype == GF_ISOM_SUBTYPE_XDVB) | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a problem with MEDIA_TYPE? I think the switch() version is cleaner and having if's.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think so, the problem might be with MEDIA_TYPE, I'll take care of the switch() version, here.
@growupboron could you link the commit you're reverting? |
@NilsIrl I just cloned the 0.86 and regressed the code. |
Closing - seems to be abandoned? |
In raising this pull request, I confirm the following (please check boxes):
My familiarity with the project is as follows (check one):
Fixed the issue by regressing the changes, as discussed on the issue page, 0.85 release was working fine, on further investigation, 0.86 release was also correct. From 0.87 release, this issue started to pop up, so regressed the code back to 0.86 release for the full functionality.